Code review comment for lp:~charclo-michael/entertainer/small-fixes

Michael-

  Thanks for the branch, and glad to see you back working on Entertainer.
We really felt overwhelmed to not have you around, and you're a great
contributor. It's been a while since you've contributed, and we've had some
workflow changes that might be helpful to know for future contributions.

  Usually, when a branch is proposed for merging, there should be two things
added in the comments for the merge proposal (ideally in the same comment): the
diff against trunk (this won't be required for very long, as Launchpad is about
to start generating this for us), and a description of what the branch does.

  On first glance at this branch, I don't notice any tests for this new
functionality. This branch can't land without tests. If you need help doing
that, I'd be glad to help, so feel free to ask. Other comments inline:

 review needsfixing

> === modified file 'entertainerlib/backend/components/mediacache/video_metadata_search.py'
> --- entertainerlib/backend/components/mediacache/video_metadata_search.py 2008-12-07 20:41:05 +0000
> +++ entertainerlib/backend/components/mediacache/video_metadata_search.py 2009-02-07 19:07:05 +0000
> @@ -25,8 +25,7 @@ class VideoMetadataSearch(threading.Thre
> # Title split keywords
> __TITLE_SPLIT_KEYWORDS = [
> "[", "]", "~", "(", ")", "dvdscr", "dvdrip", "dvd-rip", "dvdr", "vcd",
> - "divx", "xvid", "ac3", "r5", "pal", "readnfo", "uncut", "cd1", "cd2",
> - "dvdiso"
> + "divx", "xvid", "readnfo", "cd1", "cd2", "dvdiso"
> ]
>
> # Title strip items

Even "private" constants like this should not start with two underscores. It
makes it hard to test, and doesn't provide anything extra. Please only use one
underscore here.

> @@ -107,7 +106,6 @@ class VideoMetadataSearch(threading.Thre
> except:
> self.logger.error("IMDB search failed")
> return # Network error or too many results to handle
> -
> if len(search_results) == 0:
> return # No matches for this search
>
> @@ -122,11 +120,16 @@ class VideoMetadataSearch(threading.Thre
> return
>
> video_type = "MOVIE"
> +
> try:

You've got trailing whitespace here.

> title = movie['title']
> year = movie['year']
> - # convert to 5-stars rating
> - rating = round(float(movie['rating']) / 2)
> +
> + # convert to 5-stars rating if available
> + try:
> + rating = round(float(movie['rating']) / 2)
> + except:
> + rating = 0
>
> genres = ','.join(movie['genres'])
> try:

I think "5-stars rating" reads a little funny. "5-star ratings" is probably
better. Also, please no open exception catching here. Please catch a specific
exception here.

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F