Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Basic support for itunes rss tags #2674

Closed
wants to merge 7 commits into from

Conversation

@anderbubble
Copy link
Contributor

anderbubble commented Feb 19, 2017

I'm working on enhancing Nikola to support publishing a podcast RSS feed. Most of this work is going into a "postcast" plugin I'm working on, and will submit or publish eventually; but some of what I'm trying to do requires support in Nikola itself.

This PR aims to support iTunes-specific tags in RSS feeds.

Other related PRs I'm working on:

  • #2676 (copyright tag in rss feeds)
  • #2675 (explicit / static guids)

I'd also be interested in direction for where and how to publish a plugin, when that's ready.

todo

  • remove references to postcast
@@ -1272,13 +1286,66 @@ def publish_extensions(self, handler):
})
handler.endElement("atom:link")

if self.itunes_author:

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 19, 2017

Member

You could duplicate less code, eg. with getattr or some iTunes data dictionary.

@Kwpolska
Copy link
Member

Kwpolska commented Feb 19, 2017

Looks good (apart from a lot of duplicated code). Please update CHANGES.txt, AUTHORS.txt, and the manual.

self.itunes_categories = itunes_categories
self.itunes_explicit = itunes_explicit

return rss.RSS2.__init__(self, **kwargs)

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 19, 2017

Member

Don’t use return here, instead call rss.RSS2.__init__ at the start of this function. (old-style class in Python 2, so super() does not work(

@anderbubble
Copy link
Contributor Author

anderbubble commented Feb 19, 2017

First, thank you for reviewing this pull request!

I returned __init__ from __init__ because the code for Item already did. Should I clean that up at the same time?

As for the duplicated code: can you provide a pointer to somewhere else in the codebase that does something similar better? (It'd just be nice to have an example of what idiom is considered most normal in this codebase.)

I'm also not happy with my handling of the Image url. Any suggestions for how to do it better?

@Kwpolska
Copy link
Member

Kwpolska commented Feb 19, 2017

  1. Yes, you can do that as well.
  2. I don’t think we have anything like this. Here’s one example:
itunes = {'itunes:subtitle': 'foo'}
# in publish_extensions
for name in ['itunes:author', 'itunes:subtitle', 'itunes:summary', 'itunes:duration']:
    if name in itunes:
        handler.startElement(name, {})
        handler.characters(itunes[name])
        handler.endElement(name)
# special handling for image and explicit stays
  1. You could ask url_replacer with url_type='absolute'.
@@ -1649,7 +1649,7 @@ def apply_shortcodes(self, data, filename=None, lang=None, with_dependencies=Fal

def generic_rss_renderer(self, lang, title, link, description, timeline, output_path,
rss_teasers, rss_plain, feed_length=10, feed_url=None,
enclosure=_enclosure, rss_links_append_query=None):
enclosure=_enclosure, rss_links_append_query=None, itunes=None):

This comment has been minimized.

Copy link
@felixfontein

felixfontein Feb 19, 2017

Contributor

Souldn't the default value for itunes be False and not None? After all, it is only used like a boolean.

@anderbubble
Copy link
Contributor Author

anderbubble commented Feb 19, 2017

@Kwpolska
Copy link
Member

Kwpolska commented Feb 19, 2017

None is good in this case IMO.

@anderbubble anderbubble changed the title Basic support for itunes rss tags [WIP] Basic support for itunes rss tags Feb 25, 2017
@anderbubble
Copy link
Contributor Author

anderbubble commented Feb 25, 2017

I see now that I've added explicit references to my "postcast" plugin in this Nikola core PR; so I'm going to have to factor that out.

Comments welcome on the other bits, though.

Copy link
Member

Kwpolska left a comment

Looks good, apart from minor style glitches.

or post.author(lang))
args['itunes_summary'] = post.meta[lang].get('itunes_summary') or data
if 'itunes_image' in post.meta[lang]:
args['itunes_image'] = self.url_replacer(post.permalink(), post.meta[lang]['itunes_image'], lang, 'absolute')

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 25, 2017

Member

Just keep in mind that the permalink won’t affect parsing of itunes_image paths, hey should be relative to site root (in your posts).

This comment has been minimized.

Copy link
@anderbubble

anderbubble Feb 26, 2017

Author Contributor

That hasn't been my experience. With this code, the image is located relative to the post's location, and then an absolute url to that location is generated.

edit: it appears to work exactly like a standard image directive.

for tag in ('subtitle', 'duration', 'explicit'):
key = 'itunes_{}'.format(tag)
args[key] = post.meta[lang].get(key)
args['itunes_author'] = (

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 25, 2017

Member

Write that on one line, will look better and fix a style checker complaint.

# It's an old style class
rss.RSS2.__init__(self, **kwargs)

def _itunes_attributes (self):

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 25, 2017

Member

Remove the space before (self) here.

This comment has been minimized.

Copy link
@anderbubble

anderbubble Feb 26, 2017

Author Contributor

Oops. That's a minor stylistic affectation I use in my own code, but don't intend to inflict on anyone. Just a habit. :)


class ExtendedItem(rss.RSSItem):
"""Extended RSS item."""

def __init__(self, **kw):
def __init__(self, itunes_author=None, itunes_subtitle=None,

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 25, 2017

Member

It might be better to use the kw dict as well for your itunes attributes.

This comment has been minimized.

Copy link
@anderbubble

anderbubble Feb 26, 2017

Author Contributor

Doesn't that kind of reading-into-kwargs defeat static code analysis? If you don't declare what arguments you take in your function definition, you're unnecessarily hiding information.

At least, that's been my take on it in the past.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Feb 26, 2017

Member

I would prefer to keep it this way if it was started this way — perhaps there’s a reason for using kw there?

anderbubble and others added 2 commits Feb 26, 2017
@anderbubble
Copy link
Contributor Author

anderbubble commented Feb 27, 2017

I'm actually going a different way on this; leave this open for now, but I'd prefer to merge #2686 rather than this, and do my iTunes-specific stuff in my plugin rather than pollute core.

@felixfontein
Copy link
Contributor

felixfontein commented Mar 28, 2017

Since now #2686 is merged, can we close this @anderbubble?

@anderbubble
Copy link
Contributor Author

anderbubble commented Apr 24, 2017

@felixfontein for sure. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.