Skip to content
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

Docs: misleading example in prev and next record #1428

Open
capoaira opened this issue Dec 8, 2024 · 3 comments
Open

Docs: misleading example in prev and next record #1428

capoaira opened this issue Dec 8, 2024 · 3 comments
Labels

Comments

@capoaira
Copy link

capoaira commented Dec 8, 2024

Describe the bug
According to the Docs (https://django-simple-history.readthedocs.io/en/latest/querying_history.html#getting-previous-and-next-historical-record), history.frist() will return the first version of the object and therefore, prev_record will be None. But actually it is the other way around. history.frist() returns the most recent version and therefore next_record in None

Or did I misunderstand something?

Expected behavior
The documentation is not clear about it, which leads to errors.

Environment (please complete the following information):

  • Django Simple History Version: 3.5.0
  • Django Version: 4.2.11
  • postgres: 15
@jnovinger
Copy link
Member

jnovinger commented Dec 11, 2024

I started to answer with an explanation of how this worked and then realized I had misinterpreted the example code as well. So, yeah, agreed the docs could perhaps be a little more clear, perhaps some code comments in the example code.


@capoaira , yes, I do think you are misunderstanding how .first(), .last() and prev_record/next_record work. The Django docs (linked for Django 4.2) say that .first() returns the first item in the queryset and .last() returns the last item in the queryset. As the note says, it's important to understand how the QuerySet instance is sorted, either via Models.Meta.ordering or via an explicit .order_by() queryset operation.

Returns the first object matched by the queryset, or None if there is no matching object. If the QuerySet has no ordering defined, then the queryset is automatically ordered by the primary key.

Also note that .first() and .last() come from the Django QuerySet API and don't appear to be re-defined in django-simple-history at all. On the other hand, prev_record and next_record are provided by django-simple-history via the simple_history.models.HistoricalRecords API. These implementations specifically order by the history_date field, which may be very different than what the original queryset in your own code is ordered by (as noted above). That would definitely cause some confusion.

It's been a while since I've worked with django-simple-history and all of the above is coming from memory and reading the code, so I may be wrong! I'll try to spin up a test environment as soon as I can and verify what I said.

@capoaira
Copy link
Author

Thank you, @jnovinger.
After I realised that first() give me the last entered entry, I test a bit it it appears that it is exactly like you explained. Now, I'm using the earliest() method, which comes from django-simple-history and returns the first snapshot by simple history.

@capoaira capoaira changed the title Docs: Querying first and last swapped Docs: misleading example in prev and next record Dec 14, 2024
@ddabble
Copy link
Member

ddabble commented Dec 14, 2024

Well explained, @jnovinger!

I can add that I think the issue partly/mainly stems from the word "first" being slightly ambiguous, in that - in this context - it can both mean "first of a sequence" (i.e. [0]) and "first chronologically" (i.e. earliest). As @jnovinger mentioned, Django's first() method implements the former meaning. Additionally, the historical record querysets are ordered by -history_date (descending) by default, which means:

  • first() ([0]) returns the latest record
    • Recommend calling latest() instead, to reduce ambiguity
  • last() ([-1]) returns the earliest record
    • Recommend calling earliest() instead, to reduce ambiguity

I definitely agree that this should be clearer in the docs, which I'll try to improve at some point in the future - if not someone else gets to it first :)

@ddabble ddabble added the docs label Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants