Extend playlist #24

Merged
merged 6 commits into from Dec 10, 2012

Conversation

Projects
None yet
2 participants
@kracekumar
Contributor

kracekumar commented Dec 6, 2012

Added Extend playlist future.

hgtv/forms/playlist.py
@@ -40,5 +41,12 @@ class PlaylistAddForm(Form):
playlist = wtf.SelectField('Add to playlist', coerce=int)
+def playlist_validate_url(self, field):
+ youtube_regex = re.compile(r'(http|https)://www.youtube.com/playlist?([0-9A-Za-z]+.)', re.UNICODE)

This comment has been minimized.

@jace

jace Dec 6, 2012

Member

Move this to module level so that you don't recompile each time the validator is called -- that's no different from using a raw regex string. We should also establish a convention for naming regex strings. I've been using the _re suffix in other places.

@jace

jace Dec 6, 2012

Member

Move this to module level so that you don't recompile each time the validator is called -- that's no different from using a raw regex string. We should also establish a convention for naming regex strings. I've been using the _re suffix in other places.

hgtv/views/playlist.py
@@ -49,7 +49,7 @@ def inner(start_index=1, max_result=50, total=0):
# If the video is private still youtube provides the title but doesn't
# provide thumbnail & urls, check for private video
is_private = item.get('app$control')
- if is_private is not None and is_private['yt$state']['reasonCode']:
+ if is_private is not None and is_private.get('yt$state') and is_private.get('yt$state').get('reasonCode'):

This comment has been minimized.

@jace

jace Dec 6, 2012

Member

This bit doesn't make sense: is_private.get('yt$state').get('reasonCode')

The .get() call is used when the key may not exist, but if it does not exist, the following .get() will fail. Your line should be:

if is_private is not None and is_private.get('yt$state') and is_private['yt$state'].get('reasonCode'):

@jace

jace Dec 6, 2012

Member

This bit doesn't make sense: is_private.get('yt$state').get('reasonCode')

The .get() call is used when the key may not exist, but if it does not exist, the following .get() will fail. Your line should be:

if is_private is not None and is_private.get('yt$state') and is_private['yt$state'].get('reasonCode'):

This comment has been minimized.

@jace

jace Dec 6, 2012

Member

That is, you are testing for None already, so the .get() is redundant.

@jace

jace Dec 6, 2012

Member

That is, you are testing for None already, so the .get() is redundant.

hgtv/views/playlist.py
video.thumbnail_path = thumbnails.save(filestorage)
video.video_sourceid = item['media$group']['yt$videoid']['$t']
video.video_source = u"youtube"
video.make_name()
- playlist.videos.append(video)
+ if not Video.query.filter_by(video_url=video.video_url, playlist=playlist).first():

This comment has been minimized.

@jace

jace Dec 6, 2012

Member

Don't limit the query by current playlist. If the video is already in another playlist, just add it from there.

Also, video_url can vary for the same URL since Youtube sometimes adds extra characters like ?plcp=1. Look for unique video_source and video_sourceid instead.

@jace

jace Dec 6, 2012

Member

Don't limit the query by current playlist. If the video is already in another playlist, just add it from there.

Also, video_url can vary for the same URL since Youtube sometimes adds extra characters like ?plcp=1. Look for unique video_source and video_sourceid instead.

This comment has been minimized.

@kracekumar

kracekumar Dec 6, 2012

Contributor

I wrote this query to avoid duplicate videos for extend playlist(currently youtube allows duplicate videos to exist in playlist). Yes check should be for video_sourceid, now I feel check should be made before creating video object, and check for entire video list and ignore if it is in current playlist.

@kracekumar

kracekumar Dec 6, 2012

Contributor

I wrote this query to avoid duplicate videos for extend playlist(currently youtube allows duplicate videos to exist in playlist). Yes check should be for video_sourceid, now I feel check should be made before creating video object, and check for entire video list and ignore if it is in current playlist.

hgtv/views/playlist.py
@@ -79,8 +81,7 @@ def inner(start_index=1, max_result=50, total=0):
raise DataProcessingError("Unable to establish connection")
except gaierror:
raise DataProcessingError("Unable to resolve the hostname")
- except KeyError:
- raise
+ except KeyError, key:

This comment has been minimized.

@jace

jace Dec 6, 2012

Member

Shouldn't that be Keyerror, e since the second item is the exception object, not the key? Also, is it needed? Your exception handler isn't doing anything with the exception object.

@jace

jace Dec 6, 2012

Member

Shouldn't that be Keyerror, e since the second item is the exception object, not the key? Also, is it needed? Your exception handler isn't doing anything with the exception object.

This comment has been minimized.

@kracekumar

kracekumar Dec 6, 2012

Contributor

Yes, it should be e. Actually I wanted to capture particular KeyError like yt$state later figured out you can't do it. It should be e.key == 'key', will remove it.

@kracekumar

kracekumar Dec 6, 2012

Contributor

Yes, it should be e. Actually I wanted to capture particular KeyError like yt$state later figured out you can't do it. It should be e.key == 'key', will remove it.

hgtv/views/playlist.py
if 'media$description' in r.json['feed']['media$group']:
playlist.description = escape(r.json['feed']['media$group']['media$description']['$t'])
for item in r.json['feed'].get('entry', []):
# If the video is private still youtube provides the title but doesn't
# provide thumbnail & urls, check for private video
is_private = item.get('app$control')
- if is_private is not None and is_private['yt$state']['reasonCode']:
+ if is_private is not None and is_private.get('yt$state') and is_private['yt$state'].get('reasonCode'):

This comment has been minimized.

@jace

jace Dec 8, 2012

Member

Just realized you can replace the two lines above with this one:

if item.get('app$control', {}).get('yt$state', {}).get('reasonCode'):  # Is it private?
    continue

The comment is to explain what this is checking for since the variable names aren't intuitive.

@jace

jace Dec 8, 2012

Member

Just realized you can replace the two lines above with this one:

if item.get('app$control', {}).get('yt$state', {}).get('reasonCode'):  # Is it private?
    continue

The comment is to explain what this is checking for since the variable names aren't intuitive.

jace added a commit that referenced this pull request Dec 10, 2012

@jace jace merged commit e589bed into master Dec 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment