-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(rss): make title optional if description is provided #9610
fix(rss): make title optional if description is provided #9610
Conversation
🦋 Changeset detectedLatest commit: 1666e75 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Seems like there's a test that see if we throw an error on title, should probably be removed! |
Co-authored-by: Erika <[email protected]>
Co-authored-by: Erika <[email protected]>
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.
LGTM!
Following #9577 (comment), could we also make astro/packages/astro-rss/src/index.ts Lines 117 to 123 in 2814984
And possibly handling for undefined astro/packages/astro-rss/src/index.ts Lines 212 to 221 in 2814984
|
? result.link | ||
: createCanonicalURL(result.link, rssOptions.trailingSlash, site).href; | ||
item.link = itemLink; | ||
item.guid = { '#text': itemLink, '@_isPermaLink': 'true' }; |
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.
Since it depends on link, I add it here. But I don't know if this property is supposed to be something else when there is no link
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.
Looks great! Thanks for fixing the other fields as well.
Co-authored-by: Emanuele Stoppa <[email protected]>
return chai.expect(pagesGlobToRssItems(globResult)).to.not.be.rejected; | ||
}); | ||
|
||
it('should fail on missing "description" key if "title" is present', () => { |
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.
Minor nit: I think should fail
should be should not fail
to correctly match the behavior of the test.
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.
Yes you're right! Feel free to submit a 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.
Done. See #9646
@florian-lefebvre I was looking closer at this fix to learn from it and I think there is a typo in the description of the test. See comment review above. Minor nit. |
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
Testing
Updated tests
Docs
withastro/docs#6154