-
-
Notifications
You must be signed in to change notification settings - Fork 217
feat(atom/rss/json): support language and on feed and feed items (#196) #226
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
base: master
Are you sure you want to change the base?
Conversation
Greenheart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this! I was just about to do this myself, but was happily surprised it was already done 😄
Tests look good, pass, and the code looks good.
Some comments about related tasks, but some of them might be better suited for separate issues and PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language support for JSON feeds looks good. Thanks for implementing this!
| feedItem.language = item.language; | ||
| } | ||
|
|
||
| if (item.author) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this PR is about improving support for language, and not primarily about supporting JSON Feed v1.1.
However, JSON Feed v1.1 deprecated the top-level author in favour of the top-level authors array. It's still safe to use the author field, and technically this the output is still valid JSON Feed v1.1. However, this library doesn't yet support all of the features of the JSON Feed 1.1 specification, like the authors field which seem to be the future-proofed method.
Adding support for authors might be better to solve in a follow-up issue and separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments:
-
Since this changes the behaviour of
language, it would be good to also update theREADMEto indicate thatlanguageis now supported for all feed formats. -
Ideally, also update the README to note that it's now possible to specify
languageon individual feed items.
Making these changes as part of this PR would be ideal to avoid confusion for new users, but these changes could also be a follow-up PR to get this merged more quickly.
| if (ins.options.language) { | ||
| // Atom uses the reserved "xml:" namespace for language; | ||
| // no extra xmlns declaration is required. | ||
| feedAttrs["xml:lang"] = sanitize(ins.options.language) || ins.options.language; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why fall back to the unsanitized value if the sanitization fails or returns a falsy value? Wouldn't it be better to throw an error, or log a warning and omit the language field instead?
Overview
This PR implements comprehensive language support across all feed formats (Atom 1.0, JSON Feed 1.1, and RSS 2.0), addressing issue #196 which requested language support in Atom feeds. The implementation goes beyond the original request to provide consistent language functionality across all supported feed formats.
What's New
Atom 1.0 Language Support
xml:langattribute to<feed>element when language is specified<entry>elements can have their ownxml:langattributesJSON Feed 1.1 Enhancement
languagefieldlanguagefield to top-level feed objectRSS 2.0 Verification
<language>element supportTechnical Implementation
Type System Updates
language?: stringto theIteminterface for per-item language supportFeedOptions.languagefield was already present and workingSecurity Enhancements
sanitize()functionStandards Compliance
xml:langattributes as per W3C Atom specificationTesting
Created dedicated test files for maintainability:
Test Coverage Includes:
Language Support Matrix
xml:langxml:langlanguagelanguage<language>Usage Examples
Basic Feed-Level Language
Per-Item Language Support
Related Issues