Opened 15 years ago
Closed 13 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: |
Description
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 by , 15 years ago
- setup.py: don't depend on "gst". I think, it was decided, that optional packages have to be installed by users using virtualenv.
- mediagoblin/views.py: Why does it import media_types?
- mediagoblin/views.py: Why does it import sys and give it to root.html?
comment:2 by , 15 years ago
I commented out the requires='gst' till we find out from joar why it's there.
comment:3 by , 15 years ago
- mediagoblin/media_types/video/transcoders.py: 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__.
- mediagoblin/media_types/image/processing.py: Are all those celery imports needed?
- get_media_manager(_media_type = None): Why the default argument? I think, the function will fail with the default argument.
comment:5 by , 15 years ago
https://github.com/jwandborg/mediagoblin/tree/b681-comments_from_reviewing_video
Everything fixed except the pygst unit test issue.
comment:6 by , 15 years ago
| Owner: | changed from to |
|---|---|
| Status: | New → Feedback |
Haven't solved the failing tests yet, assigning it for feedback meanwhile.
comment:7 by , 15 years ago
| Owner: | changed from to |
|---|---|
| Status: | Feedback → In Progress |
Great! Merged!
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:9 by , 14 years ago
| Owner: | removed |
|---|---|
| Status: | accepted → assigned |
comment:10 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
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)
