Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30817 closed Cleanup/optimization (fixed)

Document that Sitemap.items() can return an iterable.

Reported by: Chris Jerdonek Owned by: Christoffer Sjöbergsson
Component: Documentation Version: dev
Severity: Normal Keywords: sitemap
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently, the Django docs don't say that Sitemap.items() can return a QuerySet. It just says it can return a list:

Sitemap.items: Required. A method that returns a list of objects.

(From: https://github.com/django/django/blob/fa8fe09e4e2b538c5d50a559081861d5c0635d55/docs/ref/contrib/sitemaps.txt#L136-L142 )

:attr:~Sitemap.items() is a method that returns a list of objects.

(From: https://github.com/django/django/blob/fa8fe09e4e2b538c5d50a559081861d5c0635d55/docs/ref/contrib/sitemaps.txt#L119 )

We know it can be a QuerySet because e.g. the items are passed to a Paginator:

https://github.com/django/django/blob/fa8fe09e4e2b538c5d50a559081861d5c0635d55/django/contrib/sitemaps/__init__.py#L80

and Paginator objects accept QuerySet objects: https://docs.djangoproject.com/en/2.2/topics/pagination/#example

Knowing that it can return a QuerySet is useful if you want to paginate your sitemap without having to temporarily query and store in memory all objects in one (possibly giant) list.

Change History (12)

comment:1 by Mariusz Felisiak, 5 years ago

Summary: Document that Sitemap.items can return a QuerySetDocument that Sitemap.items() can return an iterable.
Triage Stage: UnreviewedAccepted

Sitemap.items() can return not only a QuerySet or list but also tuple or any other sliceable object with a count() or __len__() method, however I don't think that we need to be so specific here. There is also a GenericSitemap that allows you to create a sitemap by passing it a dictionary which has to contain a queryset entry. I would suggest to change list to a sequence, e.g.

A method that returns a :term:`sequence` of objects.

comment:2 by Christoffer Sjöbergsson, 5 years ago

Has patch: set

comment:3 by Chris Jerdonek, 5 years ago

If it's going to say "sequence," I would suggest explicitly listing QuerySet as an example (e.g. by adding "including QuerySet" afterwards). The reason is that I don't think it's obvious whether or not a QuerySet is considered a sequence, and QuerySet is the specific thing I had a question about. For example, if the reader looks up Python's definition of a sequence, it's not clear whether QuerySet supports all of the "common sequence operations": https://docs.python.org/3/library/stdtypes.html#common-sequence-operations Like, concatenation is one of the operations, and concatenation isn't documented in the QuerySet API: https://docs.djangoproject.com/en/dev/ref/models/querysets/

comment:4 by Mariusz Felisiak, 5 years ago

QuerySet don't need to support these operation to be considered as a sequence, please take a look at term-sequence. Moreover changed paragraph is about "an example" with QuerySet. That's why using sequence sounds sufficient to me.

comment:5 by Chris Jerdonek, 5 years ago

Especially since QuerySet is a common case, it seems better to state it explicitly rather than leaving it as a puzzle for the reader to figure out.

Even if the reader clicks on the glossary entry, the answer still isn't clear and leaves a subtle, nuanced questions. For example, the glossary entry says "an iterable which supports efficient element access using integer indices," but QuerySet objects don't support negative indices. So does that mean QuerySet objects aren't included? Rather than the reader having to sort through questions like these, the docs can be more helpful by simply stating it.

comment:6 by KESHAV KUMAR, 5 years ago

Owner: changed from nobody to KESHAV KUMAR
Status: newassigned

comment:7 by Mariusz Felisiak, 5 years ago

Owner: changed from KESHAV KUMAR to Christoffer Sjöbergsson

KESHAV, this issue has a patch already.

comment:8 by KESHAV KUMAR, 5 years ago

Can you tell me why it is still open ?

Last edited 5 years ago by KESHAV KUMAR (previous) (diff)

comment:9 by Mariusz Felisiak, 5 years ago

Because patch is not merged.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 516200c0:

[3.0.x] Fixed #30817 -- Clarified return value of Sitemap.items().

Backport of 7b3c06cd72e691ffd932ccce338701c37297a415 from master

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 7b3c06cd:

Fixed #30817 -- Clarified return value of Sitemap.items().

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In da31472a:

[2.2.x] Fixed #30817 -- Clarified return value of Sitemap.items().

Backport of 7b3c06cd72e691ffd932ccce338701c37297a415 from master

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