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

Add Support for JSON Feed v1 #56

Merged
merged 1 commit into from
Jun 3, 2017
Merged

Conversation

benmccormick
Copy link
Contributor

This PR adds support for v1 of JSON Feed: https://jsonfeed.org/

It's still pretty early days for JSON Feed but feed readers are starting to support it and it was fairly straightforward to add it as an option for feed.

Things to note from this PR:

  1. I added 2 new top level options, favicon and feedLinks. The JSON feed spec heavily encourages including a link to the feed url and specifying a separate favicon and site image. I couldn't find analogues to these things in RSS2.0, but Atom supports both, so I made the changes to atom so setting a favicon and an image will work for both atom and JSON Feed. For feedLinks, there was an existing options.feed that seemingly assumed that it was an atom feed link. Since a user might presumably want to generate both an atom and a JSON feed (since JSON feed will have limited support for a while), I felt it was necessary to add a new config object that could take both JSON and atom feed links. I maintained the existing behavior though, and the atom link will get pulled from options.feed if its available, but fallback options.feedLinks.atom. If you're on board with this, a deprecation message could possibly be added for options.feed.

  2. Not all concepts that feed supports are handled by JSON feed. Contributor lists, multiple authors, and "categories" don't map cleanly to JSON Feed. I ignored most of those, but clipped authors down to the first author if multiple were given.

  3. I fixed a typo in the README and added examples for the new fields

  4. I parsed to JSON before testing the JSON feed output, because Jest will give nice diffs if JSON objects are different that it wouldn't be smart enough to do right if a string comparison were wrong.

I'm happy to clean up anything you're concerned about here, just let me know what you think.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 89.503% when pulling a0c835e on benmccormick:master into 2810732 on jpmonette:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 89.503% when pulling f1ce790 on benmccormick:master into b68d4cc on jpmonette:master.

@jpmonette jpmonette merged commit df2a295 into jpmonette:master Jun 3, 2017
@jpmonette
Copy link
Owner

Thanks for your work @benmccormick - just merged things in the repo.

In case you do another PR in the future, make sure to split it in multiple commits for each of the points you enumerated above. It would be easier to link the change with a description instead of all changes in one commit.

@benmccormick
Copy link
Contributor Author

@jpmonette great thanks for merging! I will keep that in mind for the future.

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

Successfully merging this pull request may close these issues.

3 participants