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

generic_rss_renderer refactor #2686

Merged

Conversation

@anderbubble
Copy link
Contributor

@anderbubble anderbubble commented Feb 27, 2017

Pull Request Checklist

  • I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • I tested my changes.

Description

I split actual feed generation into generic_rss_feed and writing into utils.rss_writer, both of which are now called from the (much smaller) generic_rss_renderer. This allows plugins to use generic_rss_feed, modify the resultant feed, and then write it out with utils.rss_writer.

In my use case, I'm building a "postcast" plugin to generate podcasts from posts. I want to add iTunes-specific tags to the podcast RSS feeds, but I didn't like the feeling of bolting domain-specific tags into the central rss renderer. My WIP plugin can now use the upstream code, but modify the result before it's written out to a file.

I've further made a small change to ExtendedItem to make it easier to subclass, as I use subclasses of ExtendedRSS2 and ExtendedItem to actually apply the iTunes tags.

anderbubble added 2 commits Feb 19, 2017
Split out separate generic_rss_feed and utils.rss_writer from
generic_rss_renderer. This allows generic_rss_feed and utils.rss_writer
to be used by plugins to generate and write out rss feeds, but to modify
them between initial generation and writing.
Using kw.pop to get a `creator` kwarg from ExtendedItem.__init__
makes `creator` required, but later code treats it as potentially
False or None. Allow it to actually be optional (with default value
None) to make ExtendedItem easier to subclass.
@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Feb 27, 2017

I'll add some docstrings later. Let me know if this kind of internal change justifies an entry in CHANGES.txt.

@ralsina
Copy link
Member

@ralsina ralsina commented Mar 4, 2017

Sure, you can put in CHANGES something like:

  • Refactoring allowing plugins better access to feed generation

Also, remember to add yourself to authors if you are not there.

@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Mar 5, 2017

OK. I've added docstrings and updated CHANGES. (I'm already in AUTHORS from a previous merge.)

As an aside, I'd love for someone to look over the related plugin work I'm doing, too; but I don't know that a PR is the best place for it. Any advice?

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Mar 5, 2017

Is your current plugin code available somewhere on github?

@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Mar 5, 2017

@felixfontein not yet, but I just found the appropriate documentation, so I'll do that soon.

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Mar 5, 2017

You can also create a new repository in your own github account and put it there; but forking and creating a PR for getnikola/plugins is of course fine, too.

@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Mar 6, 2017

@felixfontein It's not done, but here's what I'm ultimately working on: https://github.com/anderbubble/nikola-postcast

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Mar 11, 2017

Both PR and your plugin look fine to me.

(In the plugin, you seem to ignore the SHOW_UNTRANSLATED_POSTS setting and you might also want to check post.is_post before including it into the feed to exclude pages.)

Copy link
Contributor

@felixfontein felixfontein left a comment

LGTM

@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Mar 11, 2017

@felixfontein thanks for the comments. I've created anderbubble/nikola-postcast#5 and anderbubble/nikola-postcast#6 to track your suggestions.

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Mar 11, 2017

Apropos: do you want to leave this PR open a bit more, or do you want to see it merged soon?

@anderbubble
Copy link
Contributor Author

@anderbubble anderbubble commented Mar 11, 2017

@Kwpolska Kwpolska merged commit 958ff91 into getnikola:master Mar 25, 2017
0 of 3 checks passed
0 of 3 checks passed
codacy/pr Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Mar 25, 2017

Merged because it can be useful for others, and Atom works completely differently anyway.

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

4 participants