Opened 10 years ago

Closed 9 years ago

#332 closed defect (fixed)

Comments from reviewing the new video merge

Reported by: Elrond Owned by:
Priority: minor Milestone:
Component: programming Keywords:
Cc: Parent Tickets:


In this bug I will put comments as I review the new video code. I
will add new comments to this ticket as I go. I hope this is okay
and will be workable.


Change History (10)

comment:1 Changed 10 years ago by Elrond

1) don't depend on "gst". I think, it was decided, that
   optional packages have to be installed by users using virtualenv.
2) ``mediagoblin/``: Why does it import ``media_types``?
3) ``mediagoblin/``: Why does it import ``sys`` and give it
   to root.html?

comment:2 Changed 10 years ago by Christopher Allan Webber

I commented out the requires='gst' till we find out from joar why
it's there.

comment:3 Changed 10 years ago by Elrond

4) ``mediagoblin/media_types/video/``:
   ``VideoThumbnailer.errors`` is a class level variable, so it is
   shared between all instances. I don't think, thit is, what you
   want. initialize it in ``__init__``.
5) ``mediagoblin/media_types/image/``: Are all those
   celery imports needed?
6) ``get_media_manager(_media_type = None)``: Why the default
   argument? I think, the function will fail with the default

comment:4 Changed 10 years ago by Elrond

7) If one doesn't have pygst installed, unit tests fail.

comment:5 Changed 10 years ago by joar\_from\_reviewing\_video

Everything fixed except the pygst unit test issue.

comment:6 Changed 10 years ago by joar

Owner: changed from Joar Wandborg to Elvenlord Elrond
Status: NewFeedback
Haven't solved the failing tests yet, assigning it for feedback

comment:7 Changed 10 years ago by Elrond

Owner: changed from Elvenlord Elrond to Joar Wandborg
Status: FeedbackIn Progress

Leaves us with point 7).

The problem is basicly that nose imports every .py file it finds on
its way to search for unit tests in that .py file. And just
importing the file breaks.

One option would be to somehow (I have no idea how) tell nose to
search for tests only in ``mediagoblin.tests``.
Another option would be for the failing file (I can dig details up)
to check if video is enabled, and if not, cancel the parsing early
and get out (no idea either on how to do that sys.exit is not a
good idea, I guess.

comment:8 Changed 10 years ago by Will Kahn-Greene

The original url for this bug was .

comment:9 Changed 9 years ago by joar

Owner: joar deleted
Status: acceptedassigned

comment:10 Changed 9 years ago by Elrond

Resolution: fixed
Status: newclosed

The final remaining point "7)" is covered in its own ticket: #355

So let's close this one as "all points done, or at least delegated".

(hinted by ShawnRisk)

Last edited 9 years ago by Elrond (previous) (diff)
Note: See TracTickets for help on using tickets.