GNOME Bugzilla – Bug 764816
lua-factory: Add iTunes Podcast source
Last modified: 2016-05-14 00:41:07 UTC
.
Created attachment 325629 [details] [review] lua-factory: Add iTunes Podcast source A source to browse and search iTunes podcasts. XXX There are a couple of FIXMEs however
Created attachment 325630 [details] [review] lua-factory: Add iTunes Podcast source A source to browse and search iTunes podcasts. XXX There are a couple of FIXMEs however
Created attachment 325732 [details] [review] lua-factory: Add iTunes Podcast source A source to browse and search iTunes podcasts. XXX There are a couple of FIXMEs however
Created attachment 326095 [details] [review] lua-factory: Add iTunes Podcast source A source to browse and search iTunes podcasts. XXX There are a couple of FIXMEs however
Review of attachment 326095 [details] [review]: ::: src/lua-factory/sources/grl-itunes-podcast.lua @@ +24,3 @@ +ITUNES_SEARCH_PODCAST_URL = 'https://itunes.apple.com/search?term=%s&entity=podcast&country=%s&limit=%d' +ITUNES_PODCAST_URL = 'https://itunes.apple.com/%s/rss/toppodcasts/limit=%d/json' +MAX_ITEMS = 200 Why 200 is the maximum? @@ +34,3 @@ + name = "iTunes Podcast", + description = "iTunes Podcast", + supported_keys = { 'artist', 'thumbnail', 'id', 'title', 'region', 'external-url', 'genre', 'modification-date', 'mime-type', 'description' }, Although we don't have any code style for lua souces, let's try to avoid +100 chars in a single line if we can avoid it @@ +39,3 @@ + }, + tags = { 'podcast', 'net:internet' }, +} no icon? @@ +57,3 @@ +-- for i in table:gmatch('<p>([A-Z][A-Z])</p>') do +-- io.write ('"' .. i .. '"') +-- io.write (', ') Maybe break the line here every ~14 values? @@ +93,3 @@ + -- FIXME :( + grl.callback() + return It should be fixed by https://bugzilla.gnome.org/show_bug.cgi?id=764141, right? I don't mind keeping the FIXME pointing to the bug above plus a grl.debug. @@ +108,3 @@ + -- FIXME :( + grl.callback() + return ditto @@ +151,3 @@ + end + media.modification_date = result["im:releaseDate"].label + if result.summary then media.description = result.summary.label end I don't find lua's ternary option that bad... ;) > media.description = result.summary and result.sumary.label or nil @@ +156,3 @@ + + count = count - 1 + grl.debug ('Sending out media ' .. media.id .. ' (external url: ' .. media.external_url .. ' left: ' .. count .. ')') You might want to use tostring() for media.external_url to avoid nil values * we probably want to extend grl.debug to accept args. @@ +192,3 @@ + if result.artworkUrl60 then table.insert(media.thumbnail, result.artworkUrl60) end + if result.artworkUrl100 then table.insert(media.thumbnail, result.artworkUrl100) end + if result.artworkUrl600 then table.insert(media.thumbnail, result.artworkUrl600) end You might want to go for large to small. It was interesting for lastfm-cover so it might be the case here as well https://git.gnome.org/browse/grilo-plugins/commit/?id=ae252b6580037da6493734a34ce3738b02ca14b9 @@ +202,3 @@ + + count = count - 1 + grl.debug ('Sending out media ' .. media.id .. ' (external url: ' .. media.external_url .. ' left: ' .. count .. ')') You might not have media.external_url > (grilo-test-ui-0.3:11962): grl-itunes-podcast.lua:204: attempt to concatenate a nil value (field 'external_url')
(In reply to Victor Toso from comment #5) > Review of attachment 326095 [details] [review] [review]: > > ::: src/lua-factory/sources/grl-itunes-podcast.lua > @@ +24,3 @@ > +ITUNES_SEARCH_PODCAST_URL = > 'https://itunes.apple.com/search?term=%s&entity=podcast&country=%s&limit=%d' > +ITUNES_PODCAST_URL = > 'https://itunes.apple.com/%s/rss/toppodcasts/limit=%d/json' > +MAX_ITEMS = 200 > > Why 200 is the maximum? Because that's the maximum that the service will send back. I've added a comment. > @@ +34,3 @@ > + name = "iTunes Podcast", > + description = "iTunes Podcast", > + supported_keys = { 'artist', 'thumbnail', 'id', 'title', 'region', > 'external-url', 'genre', 'modification-date', 'mime-type', 'description' }, > > Although we don't have any code style for lua souces, let's try to avoid > +100 chars in a single line if we can avoid it I have wide screens, and I thought it was easier to read. But I've added a > @@ +39,3 @@ > + }, > + tags = { 'podcast', 'net:internet' }, > +} > > no icon? Added one > @@ +57,3 @@ > +-- for i in table:gmatch('<p>([A-Z][A-Z])</p>') do > +-- io.write ('"' .. i .. '"') > +-- io.write (', ') > > Maybe break the line here every ~14 values? This would complicate this generation code for no reason, IMO. It's not very pretty, but it's not like somebody will need to edit it. > @@ +93,3 @@ > + -- FIXME :( > + grl.callback() > + return > > It should be fixed by https://bugzilla.gnome.org/show_bug.cgi?id=764141, > right? > I don't mind keeping the FIXME pointing to the bug above plus a grl.debug. No, that's not the problem. The problem is that we went above what the web service offers. Not sure how we're supposed to send feedback here. I'll think of something. > @@ +108,3 @@ > + -- FIXME :( > + grl.callback() > + return > > ditto > > @@ +151,3 @@ > + end > + media.modification_date = result["im:releaseDate"].label > + if result.summary then media.description = result.summary.label end > > I don't find lua's ternary option that bad... ;) > > media.description = result.summary and result.sumary.label or nil I actually have trouble parsing that :/ > @@ +156,3 @@ > + > + count = count - 1 > + grl.debug ('Sending out media ' .. media.id .. ' (external url: ' .. > media.external_url .. ' left: ' .. count .. ')') > > You might want to use tostring() for media.external_url to avoid nil values It's not supposed to be nil, ever. > * we probably want to extend grl.debug to accept args. > > @@ +192,3 @@ > + if result.artworkUrl60 then table.insert(media.thumbnail, > result.artworkUrl60) end > + if result.artworkUrl100 then table.insert(media.thumbnail, > result.artworkUrl100) end > + if result.artworkUrl600 then table.insert(media.thumbnail, > result.artworkUrl600) end > > You might want to go for large to small. It was interesting for lastfm-cover > so it might be the case here as well > https://git.gnome.org/browse/grilo-plugins/commit/ > ?id=ae252b6580037da6493734a34ce3738b02ca14b9 Ha, sure. Done. > @@ +202,3 @@ > + > + count = count - 1 > + grl.debug ('Sending out media ' .. media.id .. ' (external url: ' .. > media.external_url .. ' left: ' .. count .. ')') > > You might not have media.external_url > > (grilo-test-ui-0.3:11962): grl-itunes-podcast.lua:204: attempt to concatenate a nil value (field 'external_url') Yeah, that was a typo, it's supposed to be .url.
Created attachment 327645 [details] [review] lua-factory: Add iTunes Podcast source A source to browse and search iTunes podcasts.
Attachment 327645 [details] pushed as 271fae4 - lua-factory: Add iTunes Podcast source