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 👍

docs/ref/contrib/syndication.txt

django/utils/feedgenerator.py

sarahboyce · 2024-05-14T08:05:19Z

docs/ref/utils.txt

+
+.. versionadded:: 5.1
+
+.. class:: Stylesheet


I realise this is how it is documented below, but we mostly don't document the __init__ method this way and do things like:

Suggested change

.. class:: Stylesheet

.. class:: Stylesheet(url, mimetype="", media="screen")

And then define each of the arguments like:

.. attribute:: url The URL of the stylesheet. This argument is required.

I quite like this as you can then link to the specific arguments.
Example: https://docs.djangoproject.com/en/5.0/ref/contrib/postgres/aggregates/#arrayagg

You're right, it's much more usable that way. One thing to note (it might not be a huge deal but I thought I'd flag it just in case): while media is indeed a true attribute, the other two are @property so if someone ever tries to instantiate a Stylesheet then change the attributes afterwords then they'll get an error. It should be possible to add setters to make the attributes work transparently, but I wasn't sure if it was worth the added complexity. What do you think?

I might be wrong, but I think we do this mostly for formatting rather than to tell the user it is an attribute vs a property 🤔 so I don't think the docs should drive any requirement for setters

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.

sarahboyce · 2024-05-15T14:52:55Z

docs/ref/contrib/syndication.txt

+        stylesheets = "/link/to/stylesheet.xsl"
+        stylesheets = feedgenerator.Stylesheet("/link/to/stylesheet.xsl")
+        stylesheets = ["/stylesheet1.xsl", "stylesheet2.xls"]
+        stylesheets = [
+            feedgenerator.Stylesheet("/stylesheet1.xsl"),
+            feedgenerator.Stylesheet("/stylesheet2.xsl"),
+        ]


I think you've documented the different types in the feed stylesheet docs so I would pick your favourite here as I think the provide one of the following three. comment doesn't quite work and I think this is more to document the _get_dynamic_attr behaviour 🤔

sarahboyce · 2024-05-15T14:56:10Z

docs/ref/contrib/syndication.txt

+
+    Beware that some control characters are
+    `not allowed <https://www.w3.org/International/questions/qa-controls>`_ in
+    XML documents. If your content has some of them, you might encounter a
    :exc:`ValueError` when producing the feed.


I think we're supposed to add this here as well maybe

.. versionchanged:: 5.1 The ``stylesheets`` argument was added.

sarahboyce · 2024-05-15T14:57:26Z

docs/ref/contrib/syndication.txt

+You can add styling to your RSS feed using a stylesheet (typically in the XSLT_
+or CSS formats) by setting the ``stylesheets`` attribute on the feed class.
+
+This can be a hardcoded URL::


I think you can use .. code-block:: python to make these sections prettier

sarahboyce · 2024-05-15T15:01:27Z

docs/releases/5.1.txt

@@ -158,7 +158,9 @@ Minor features
 :mod:`django.contrib.syndication`
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-* ...
+* All :class:`~django.utils.feedgenerator.SyndicationFeed` classes now support
+  a ``stylesheet`` attribute. If specified, an ``<? xml-stylesheet ?>``


Suggested change

a ``stylesheet`` attribute. If specified, an ``<? xml-stylesheet ?>``

a ``stylesheets`` attribute. If specified, an ``<? xml-stylesheet ?>``

sarahboyce · 2024-05-15T15:16:22Z

docs/ref/utils.txt

+
+.. versionadded:: 5.1
+
+.. class:: Stylesheet


I might be wrong, but I think we do this mostly for formatting rather than to tell the user it is an attribute vs a property 🤔 so I don't think the docs should drive any requirement for setters

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 👍

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

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

sarahboyce May 14, 2024

bmispelon May 15, 2024

sarahboyce May 15, 2024

sarahboyce May 14, 2024

sarahboyce May 15, 2024

sarahboyce May 15, 2024

sarahboyce May 15, 2024

sarahboyce May 15, 2024

sarahboyce May 15, 2024

sarahboyce May 15, 2024

	.. class:: Stylesheet
	.. class:: Stylesheet(url, mimetype="", media="screen")

	a ``stylesheet`` attribute. If specified, an ``<? xml-stylesheet ?>``
	a ``stylesheets`` attribute. If specified, an ``<? xml-stylesheet ?>``

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

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

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