Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#19904 closed Cleanup/optimization (wontfix)

Rework syndications contrib to use generic CBV's

Reported by: Mathijs de Bruin Owned by: Mathijs de Bruin
Component: contrib.syndication Version: master
Severity: Normal Keywords: django-sprint cbv syndication
Cc: mathijs@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aymeric Augustin)

Currently, the Feeds contrib is using some sort of custom CBV implementation.

Using Django's 'new' generic class based views would be beneficial in several ways:

  1. The API would be the consistent with what people are used to.
  2. It would be easier to extend the Feed API in a consistent way.
  3. It would reduce the amount of custom code.

Fixing this would probably include, or enable, fixing of #13896 , #13896 and #18112.

Change History (9)

comment:1 Changed 5 years ago by Aymeric Augustin

Description: modified (diff)

comment:2 Changed 5 years ago by Sasha Romijn

Triage Stage: UnreviewedAccepted

This sounds like a completely sensible refactoring, but I'm not very familiar with contrib.feeds. I would make sure to look into the reasons behind this design. Was this set up before CBV were introduced? Or is there some other reason for the design?

comment:3 in reply to:  2 Changed 5 years ago by Mathijs de Bruin

From looking at the code, it seems that the larger part of the work is done in django.utils.feedgenerator. Within Django it seems that this code is only used in the syndication contrib. Moreover, in practise, it seems that more often than not, any interesting sort Feed view subclass needs to subclass a SyndicationFeed as well.

Would it be a sensible decision to move all syndication code into a single (sub)class within the contrib folder, such that only a single subclass needs to be made when overriding the way feeds are being rendered?

comment:4 Changed 5 years ago by Mathijs de Bruin

Keywords: django-sprint cbv syndication added
Triage Stage: AcceptedDesign decision needed

Some idea of the kind of difficulty one has to go through to extend a Feed in practise:

Some idea on what the final API might look like:

comment:5 Changed 5 years ago by Luke Plant

I'm not convinced by the reasons given for this refactoring. The API for the Feed class evolved from the things you need for generating Atom/RSS feeds (before generic CBVs existed), and the API of View evolved from the things you need for generating HTML based views. It isn't clear to me that these will necessarily converge.

There is so little in common, that the consistent API is limited to the methods that the Feed implementer will not usually override or call directly - e.g. as_view.

I personally think that custom CBVs are a good thing - the generic CBVs that come with Django are helpful shortcuts for a significant fraction of problems, but not all problems. I'm as sceptical of this change as the attempt to use generic CBVs for the admin, which also has it's own API that has evolved from real-world needs.

comment:6 Changed 5 years ago by Aymeric Augustin

See also #11939.

comment:7 Changed 5 years ago by Jacob

Resolution: wontfix
Status: newclosed

I agree with Luke - if we were writing the feed framework now we might use CBVs. But then again we might not, and either way it's not particularly worth our effort to "port" the view over.

comment:8 Changed 5 years ago by Mathijs de Bruin

Perhaps the original ticket was not sufficiently clear:

  1. The current API for (extending) Feeds is a real pain in the buttocks and there is no common practise for doing so. Hence, a lot of apps which actively use Feeds tend to write similar extension code or write their own Feed views altogether. This is not DRY at all; this causes a lot of double work both in terms of initial development as well as maintenance. Hence the proposal could be read as: refactor the feeds API.
  1. As jacob says, if we were to rewrite the Feeds API now it *might* make sense to use generic CBV's. As both luke and jacob stress; there is not much in common apart from the as_view() call as far as *usage* is considered. However, especially with feeds, a very large part of the users will want to extend the functionality and from an 'inner' API level having a similar structure to the generic CBV's might make a lot of sense. Think of passing in optional arguments to as_view() but also overriding get_queryset(). Possible use cases include using view mixins for both generic list views as well as feeds. Wait, let me say that again: *view mixins*.

Perhaps a better description for this ticket would be: 'Refactor feeds API'.
And then as description:

Currently the functionality and (internal) API for the feeds contrib is spread out over a core module django.utils.feedgenerator and a contrib module. The original purpose for this is unclear and it makes the API hard to extend and adds code to the Django core which is actually only used by the feeds contrib. Hence the proposal reads:

  1. Factor the feedgenerator API out of django.utils and merge it into the feeds contrib such that for extending a view only a single object needs to be subclassed.
  2. While refactoring, use applicable generic CBV base classes/mixins so as the yield a common API and allow for the usage of common mixins between feed and list-type views.

Let me know if the latter makes sense to you; I could open up another ticket if you think it does (or perhaps we could update the exising ticket as not to loose context - though I'm a mere contributor and don't seem to be able to update the original ticket).

comment:9 Changed 5 years ago by Luke Plant

I think you should open a new ticket for this.

There are good reasons for the original API for feeds. There is the low level feed generator API in django.utils.feedgenerator - for generating the XML of a specific feed format. It is entirely possible to use this in projects without ever using the high level API. This code is not "only used by the feeds contrib" - it is a publicly documented API in its own right. So I would be -1 on merging that into the high level API.

I don't know in what ways you find the API hard to extend, but I think they should be the subject of another ticket, or perhaps several tickets.

Also, I think your terminology is a bit confusing - usually you "refactor" an implementation i.e. leave the API alone, and change the internals. Or, you change an API (by introducing a new one, and deprecating the old one, in the case of Django). I'm not sure which you are suggesting.

If you are proposing a new feed API, it will require significant justification. If you have very custom needs that the current high level API is ill-suited for, it is entirely possible to write your own high level API. The argument "it's not DRY" can be used to include everything possible in Django, which we will always resist. It has to be something that is demonstrated to be a common need, and where having external, possibly competing, solutions is something which is either impossible or bad for Django users.

Note: See TracTickets for help on using tickets.
Back to Top