Fixed #12978 -- Added support for RSS feed stylesheets. #18120

bmispelon · 2024-05-01T09:18:53Z

Trac ticket number

ticket-35420 (duplicate)
ticket-12978 (the actual original ticket)

Branch description

To simplify the implementation I decided to restrict the types of stylesheets to XSL (it lets me hardcode the type of the stylesheet), it seems to be the de-facto standard anyway.

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
I have attached screenshots in both light and dark modes for any UI changes.

docs/ref/contrib/syndication.txt

yuvadm · 2024-05-02T09:44:22Z

Nice to see this request popping up again after 14 years 😂 https://code.djangoproject.com/ticket/12978

docs/ref/contrib/syndication.txt

kezabelle · 2024-05-02T10:06:25Z

docs/ref/contrib/syndication.txt

+    from django.contrib.syndication.views import Feed
+    from django.urls import reverse
+
+
+    class FeedWithStylesheetView(Feed):
+        ...  # author, etc.
+        xsl_stylesheet_url = reverse("your-custom-view-name")


🤔 Does this example need to use reverse_lazy by convention, or is continuing to use reverse¹ going to be OK here?

Footnotes

As earlier in the document, but at method evaluation level, not at module level. ↩

django/utils/feedgenerator.py

bmispelon · 2024-05-02T13:00:38Z

Nice to see this request popping up again after 14 years 😂 https://code.djangoproject.com/ticket/12978

Oh, I didn't realize there was already a ticket for this (I did search before creating one but clearly not well enough).
Seems like my approach is quite similar to the one you suggested 14 years ago, except I only added support for a single stylesheet, and only for the XSL format.

Do you think those are reasonable limitations @yuvadm ? Your proposal was for multiple RSS stylesheets but from the little I've seen, XSL seems like a more widespread format, do you agree?

yuvadm · 2024-05-02T14:21:56Z

@bmispelon after so much time, I hardly remember why I added support for multiple stylesheets, I was probably aiming for maximum flexibility.

If I were to re-implemented, in terms of API design, I'd probably be liberal in accepting both single and iterable stylesheets in one arg, but unsure if that meets Django code standards.

Anyway nice to see great minds thinking alike 😄

nessita · 2024-05-03T19:16:52Z

Hey @bmispelon, I don't mean to rush you but wanted to mention that when this is ready for a review, please update the ticket flags in trac so it shows in the review queue. Thanks!

bmispelon · 2024-05-03T19:24:42Z

Hey @bmispelon, I don't mean to rush you but wanted to mention that when this is ready for a review, please update the ticket flags in trac so it shows in the review queue. Thanks!

Thanks for the heads up, I hadn't notice that the original ticket still had the "patch needs improvement" flag. I've squashed and rebased this PR and updated the flags on the ticket, it should be ready to review.

adamchainz

Very thorough PR, looks generally good to me, although I'm not familiar with XSLT stylesheets.

docs/releases/5.1.txt

docs/ref/contrib/syndication.txt

docs/ref/utils.txt

docs/ref/contrib/syndication.txt

sarahboyce · 2024-05-06T09:14:02Z

Do you think those are reasonable limitations @yuvadm ? Your proposal was for multiple RSS stylesheets but from the little I've seen, XSL seems like a more widespread format, do you agree?

I think a css stylesheet should also be supported 🤔

bmispelon · 2024-05-07T22:25:43Z

I'm a bit puzzled about the test failure. The failing test passes for me locally 🤔

django/utils/feedgenerator.py

tests/syndication_tests/tests.py

sarahboyce · 2024-05-08T07:24:21Z

django/contrib/syndication/views.py

@@ -160,6 +160,7 @@ def get_feed(self, obj, request):
            feed_copyright=self._get_dynamic_attr("feed_copyright", obj),
            feed_guid=self._get_dynamic_attr("feed_guid", obj),
            ttl=self._get_dynamic_attr("ttl", obj),
+            stylesheet=self._get_dynamic_attr("stylesheet", obj),


Do we want to support having multiple stylesheets?

I wanted to keep the API simple because I assume most people only really have a single stylesheet. I pushed a separate commit that adds documentation (and tests) for how to extend the Feed class to support multiple stylesheets.

Thank you 👍 that's a lovely addition
This is what I'm thinking.
If we think we might support multiple in the future, we'd want to deprecate stylesheet and add stylesheets. In that case it's easier to do now.
Otherwise, as you have documented how to do this in your own code, we'd be recommending users that it should be in your own code and that's a decision to not add it into Django.

I've added support for multiple stylesheets (I tried to contain it to a single commit so you can see the diff easier), it didn't add much complexity and your argument about future-proofing against a possible deprecation cycle is convincing.

I did opt to support both a single stylesheet and a list of them, which does make the code (and the documentation) a bit more complex, but I thought it was worth it so that we can offer a simpler API when people only want to use a single stylesheet. If you think that tradeoff is not worth it, I'm also happy to make the API list-only.

I don't have too strong of an opinion here. I personally would have just supported lists/tuples (a bit like admin inlines) but I don't mind having this. We can get another opinion 👍

@yuvadm @moan0s Do you have an opinion on whether we should allow both list/tuple + strings for the stylesheets argument, or only list/tuple?

With the name stylesheets I'd expect to pass an iterable, as a user my immediate reaction is to do stylesheets=["style.css"]. From the defensive programming side, I'd silently accept a single string as well.

I'd agree with @yuvadm here :) Thanks so much for working on this PR!

The version I've pushed now supports only list/tuples and throws a TypeError if you pass something else (str or Stylesheet). While I can see the argument for trying to be as lenient as possible in terms of what is accepted, I couldn't find examples in the Django codebase that did that, and I'm hesitant to have an undocumented API.

With this approach I figured it's easier to make the function more flexible if we feel the need for it in the future (since we won't have to go through a deprecation period).

Do you think that's acceptable?

docs/ref/contrib/syndication.txt

django/utils/feedgenerator.py

docs/ref/utils.txt

docs/ref/contrib/syndication.txt

sarahboyce · 2024-05-14T08:23:04Z

django/contrib/syndication/views.py

@@ -160,6 +160,7 @@ def get_feed(self, obj, request):
            feed_copyright=self._get_dynamic_attr("feed_copyright", obj),
            feed_guid=self._get_dynamic_attr("feed_guid", obj),
            ttl=self._get_dynamic_attr("ttl", obj),
+            stylesheet=self._get_dynamic_attr("stylesheet", obj),


Thank you 👍 that's a lovely addition
This is what I'm thinking.
If we think we might support multiple in the future, we'd want to deprecate stylesheet and add stylesheets. In that case it's easier to do now.
Otherwise, as you have documented how to do this in your own code, we'd be recommending users that it should be in your own code and that's a decision to not add it into Django.

docs/ref/contrib/syndication.txt

docs/releases/5.1.txt

docs/ref/utils.txt

sarahboyce · 2024-05-15T15:18:04Z

django/contrib/syndication/views.py

@@ -160,6 +160,7 @@ def get_feed(self, obj, request):
            feed_copyright=self._get_dynamic_attr("feed_copyright", obj),
            feed_guid=self._get_dynamic_attr("feed_guid", obj),
            ttl=self._get_dynamic_attr("ttl", obj),
+            stylesheet=self._get_dynamic_attr("stylesheet", obj),


I don't have too strong of an opinion here. I personally would have just supported lists/tuples (a bit like admin inlines) but I don't mind having this. We can get another opinion 👍

django/utils/feedgenerator.py

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from accd5e1 to 06bd3c7 Compare May 1, 2024 09:30

bmispelon mentioned this pull request May 1, 2024

Fixed mismatched blacken-docs behavior between CI and pre-commit #18121

Merged

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from e258b5e to 4236ef0 Compare May 1, 2024 10:40

felixxm reviewed May 1, 2024

View reviewed changes

docs/ref/contrib/syndication.txt Outdated Show resolved Hide resolved

kezabelle reviewed May 2, 2024

View reviewed changes

nessita changed the title ~~Fixed #35420: Added support for RSS feed stylesheets.~~ Fixed #12978: Added support for RSS feed stylesheets. May 3, 2024

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from c4de8d5 to 66e1eac Compare May 3, 2024 19:21

adamchainz reviewed May 3, 2024

View reviewed changes

sarahboyce reviewed May 6, 2024

View reviewed changes

docs/releases/5.1.txt Outdated Show resolved Hide resolved

felixxm reviewed May 6, 2024

View reviewed changes

docs/ref/contrib/syndication.txt Show resolved Hide resolved

felixxm reviewed May 6, 2024

View reviewed changes

docs/ref/contrib/syndication.txt Show resolved Hide resolved

felixxm reviewed May 6, 2024

View reviewed changes

docs/ref/utils.txt Show resolved Hide resolved

felixxm reviewed May 6, 2024

View reviewed changes

docs/ref/utils.txt Outdated Show resolved Hide resolved

felixxm reviewed May 6, 2024

View reviewed changes

docs/ref/contrib/syndication.txt Outdated Show resolved Hide resolved

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from 66e1eac to 8ecba20 Compare May 7, 2024 22:16

sarahboyce reviewed May 8, 2024

View reviewed changes

django/utils/feedgenerator.py Outdated Show resolved Hide resolved

tests/syndication_tests/tests.py Outdated Show resolved Hide resolved

sarahboyce reviewed May 8, 2024

View reviewed changes

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from 855b5f2 to a5ecf0f Compare May 8, 2024 14:17

sarahboyce reviewed May 14, 2024

View reviewed changes

bmispelon added 4 commits May 15, 2024 14:23

Fixed django#12978: Added support for RSS feed stylesheets.

98d6125

versionadded -> versionchanged

043d243

removed start_document() extension hook

278bfd3

fixed bug in Stylesheet.__repr__()

d7a4907

Used the empty string instead of a sentinel object

57a1ee3

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch 2 times, most recently from 647fb20 to 4426a29 Compare May 15, 2024 12:34

added builtin support for multiple stylesheets

96a77c2

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from 4426a29 to 96a77c2 Compare May 15, 2024 13:01

sarahboyce reviewed May 15, 2024

View reviewed changes

Fixes after rewview

972ac8a

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from 80b927c to a32185d Compare May 17, 2024 18:11

changed API to only accept a list of stylesheets

923cb21

bmispelon force-pushed the ticket-35420-contrib-syndication-feed-stylesheet branch from a32185d to 923cb21 Compare May 17, 2024 18:15

leandrodesouzadev reviewed May 21, 2024

View reviewed changes

django/utils/feedgenerator.py Show resolved Hide resolved

Check type of stylesheets argument

98def77

felixxm changed the title ~~Fixed #12978: Added support for RSS feed stylesheets.~~ Fixed #12978 -- Added support for RSS feed stylesheets. May 21, 2024

felixxm reviewed May 21, 2024

View reviewed changes

django/utils/feedgenerator.py Outdated Show resolved Hide resolved

Fixed missing f in f-string (+tests)

6cf2c60

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #12978 -- Added support for RSS feed stylesheets. #18120

Fixed #12978 -- Added support for RSS feed stylesheets. #18120

bmispelon commented May 1, 2024 •

edited

yuvadm commented May 2, 2024

kezabelle May 2, 2024

bmispelon commented May 2, 2024

yuvadm commented May 2, 2024 •

edited

nessita commented May 3, 2024

bmispelon commented May 3, 2024

adamchainz left a comment

sarahboyce commented May 6, 2024

bmispelon commented May 7, 2024

sarahboyce May 8, 2024

bmispelon May 8, 2024

sarahboyce May 14, 2024

bmispelon May 15, 2024

sarahboyce May 15, 2024

bmispelon May 16, 2024

yuvadm May 16, 2024 •

edited

moan0s May 17, 2024

bmispelon May 21, 2024

sarahboyce May 14, 2024

sarahboyce May 15, 2024

Fixed #12978 -- Added support for RSS feed stylesheets. #18120

Are you sure you want to change the base?

Fixed #12978 -- Added support for RSS feed stylesheets. #18120

Conversation

bmispelon commented May 1, 2024 • edited

Trac ticket number

Branch description

Checklist

yuvadm commented May 2, 2024

Choose a reason for hiding this comment

Footnotes

bmispelon commented May 2, 2024

yuvadm commented May 2, 2024 • edited

nessita commented May 3, 2024

bmispelon commented May 3, 2024

adamchainz left a comment

Choose a reason for hiding this comment

sarahboyce commented May 6, 2024

bmispelon commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvadm May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmispelon commented May 1, 2024 •

edited

yuvadm commented May 2, 2024 •

edited

yuvadm May 16, 2024 •

edited