Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17813 closed New feature (fixed)

Implement an opposite method for Entry.objects.latest

Reported by: tonnzor Owned by: Anssi Kääriäinen <akaariai@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: lemaire.adrien@…, paper82 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

At the moment we have Entry.objects.latest method that efficiently gets the last entry in queryset https://docs.djangoproject.com/en/dev/ref/models/querysets/#latest

We need another handy method that would do the same but getting the first element.

I suggest to name it first or something like that.

I have attached a sample patch.

Attachments (7)

first_latest.patch (1.5 KB) - added by tonnzor 5 years ago.
first_latest_with_tests.patch (4.2 KB) - added by jimmysong 5 years ago.
added tests, fixed the fact that the model manager didn't have a first() method.
t17813.diff (17.0 KB) - added by Adrien Lemaire 5 years ago.
Added new get_first_by Objects field. More tests. Doc updated
t17813.2.diff (17.2 KB) - added by Adrien Lemaire 5 years ago.
new diff with first renamed into earliest
t17813.3.diff (14.7 KB) - added by Adrien Lemaire 5 years ago.
get_earliest_by attr related code reverted
t17813.3.2.diff (12.4 KB) - added by Adrien Lemaire 5 years ago.
t17813.4.diff (16.8 KB) - added by Adrien Lemaire 5 years ago.

Download all attachments as: .zip

Change History (33)

Changed 5 years ago by tonnzor

Attachment: first_latest.patch added

comment:1 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedDesign decision needed

What's the use case for this method?

comment:2 Changed 5 years ago by Russell Keith-Magee

Easy pickings: set
Needs documentation: set
Needs tests: set
Triage Stage: Design decision neededAccepted

Consider the case of an event log. Retrieving the most recent event has an easy shortcut:

>>> Event.objects.latest('timestamp')

but getting the first event doesn't, so you need to do it the long way:

>>> Event.object.order_by('timestamp')[0]

Retrieving the first event by itself isn't an obvious query, but something like "Get me the first event this week" would be.

For me, this is a fairly obvious piece of symmetry in the ORM API; I've wanted it to exist a few times in recent history.

Changed 5 years ago by jimmysong

added tests, fixed the fact that the model manager didn't have a first() method.

comment:3 Changed 5 years ago by jimmysong

Needs tests: unset
Resolution: fixed
Status: newclosed

First open source contribution here, FYI.

I've added a test for the first method and added a first method to the model manager. The tests pass.

There is one thing in the patch that's inconsistent and that's the get_latest_by from the _meta. It seems too hackish to leave that as the default for the first as well. I would like some direction as to how to how to solve this dilemma. Creating a corresponding get_first_by seems really odd as Entry.objects.first() would use a completely different criteria as Entry.objects.latest(). Creating a more general thing to apply to first or latest is a naming nightmare and get_first_or_latest_by is neither backwards-compatible nor easy to remember.

Can someone give me some direction on this?

comment:4 Changed 5 years ago by jimmysong

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

comment:5 Changed 5 years ago by Adrien Lemaire

Cc: lemaire.adrien@… added
Owner: changed from nobody to Adrien Lemaire
Status: reopenednew

I read your code. Tests need to be renamed, it's not LatestTests anymore but FirstOrLatestTests.
Same goes for the model.Options.get_latest_by. Don't make any sense to ask for a get_latest_by parameter when performing and first() action.

Two solutions:

  • rename get_latest_by in get_first_or_latest_by . Backward incompatible, but that options touch a very tiny bit of code, and this would be the cleanest solution.
  • add a new get_first_by field in Options that will behave like get_latest_by.

I'll add a new patch with the 2nd solution, and will change it if the core developers agree with the first one.

Changed 5 years ago by Adrien Lemaire

Attachment: t17813.diff added

Added new get_first_by Objects field. More tests. Doc updated

comment:6 Changed 5 years ago by Adrien Lemaire

Needs documentation: unset
Patch needs improvement: unset

Done. I also added more tests to insure the correct error is raised if no Meta.get_first_by or Meta.get_latest_by is defined

comment:7 Changed 5 years ago by Henrique C. Alves

For the record: a method named first is misleading. The antonym for "latest" is "earliest". first sounds too much like you're getting the first record (by PK order) instead of by an arbitrary field.

Also, having 2 different attributes on Meta is odd. I don't see why you can't just keep get_latest_by and get along without breaking everybody's code. It will be obvious earliest is reciprocal.

comment:8 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: set

You're right for first, I just went along with jimmysong implementation without thinking about that. Will change that now

For the attributes, what is odd is to add a Meta.get_latest_by in your model when you want to call a earliest(), don't you think ? It's also more flexible to have 2 separate attributes, therefore I finally think it's the best approach. And it doesn't break everybody's code.

Changed 5 years ago by Adrien Lemaire

Attachment: t17813.2.diff added

new diff with first renamed into earliest

comment:9 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: unset

Done.

comment:10 Changed 5 years ago by Henrique C. Alves

Replying to Fandekasp:

For the attributes, what is odd is to add a Meta.get_latest_by in your model when you want to call a earliest(), don't you think ?

I don't think so. What I would consider odd, though, is having a second, analagous property, get_earliest_by, when you can just derive one from the other and stay DRY. As long as ealiest() is explained in the docs as doing the opposite of latest(), there's no more room for confusion.

It's also more flexible to have 2 separate attributes, therefore I finally think it's the best approach.

I'm afraid this is a case were developers expect behaviour to be more obvious than flexible. Allowing latest() and earliest() to return different results is guaranteed to be a source of stupid bugs, while having no valid use case.

comment:11 Changed 5 years ago by Adrien Lemaire

Owner: Adrien Lemaire deleted

I see your point. Let's wait for a core developer comment

comment:12 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

Thanks for the patch. Like hcarvalhoalves, I think it would be non-DRY and potentially confusing to allow for two attributes. get_latest_by is sufficient to indicate which field should be used for ordering, so there is no need for an extra attribute. Also, renaming get_latest_by would be unnecessarily backwards-incompatible. So, all that's needed is a new earliest() method that uses the inverse order.

Changed 5 years ago by Adrien Lemaire

Attachment: t17813.3.diff added

get_earliest_by attr related code reverted

comment:13 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: unset

done

comment:14 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

The patch looks pretty good. There's still one occurrence of 'get_earliest_by' in the tests. Could you remove it? When applying the patch I've got some warnings that there were trailing whitespaces. If you could remove those as well, that would be great. Thanks!

Changed 5 years ago by Adrien Lemaire

Attachment: t17813.3.2.diff added

comment:15 Changed 5 years ago by Adrien Lemaire

Sorry for the occurence of 'get_earliest_by' in the tests. I'm pretty sure I fixed it, so I messed up somewhere when creating the diff. Fixed now.

@julien, what are you using to get those trailing whitespaces warnings ?
I'm using flake8 usually, but I gave up on the django project:

$ flake8 .|wc
20744  148974  1795831
$ flake8 .|grep "trailing whitespace"|wc
407    1628    28496

There are 407 trailing whitespace around the repo ... Anyways, I think I fixed the ones in the files I've edited.

Cheers

Last edited 5 years ago by Adrien Lemaire (previous) (diff)

comment:16 Changed 5 years ago by Julien Phalip

I'm getting a failure when running the tests:

(djangocore-trunk)➜  tests  python runtests.py --settings=test_sqlite get_earliest_or_latest
Creating test database for alias 'default'...
Creating test database for alias 'other'...
.F.
======================================================================
FAIL: test_latest (modeltests.get_earliest_or_latest.tests.EarliestOrLatestTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/julien/Documents/Development/eclipse/workspace/DjangoCore/hg-trunk/tests/modeltests/get_earliest_or_latest/tests.py", line 65, in test_latest
    self.assertRaises(Article.DoesNotExist, Article.objects.latest)
AssertionError: _earliest_or_latest() requires either a field_name parameter or 'get_latest_by' in the model

----------------------------------------------------------------------
Ran 3 tests in 0.010s

FAILED (failures=1)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

Also, shouldn't the folder modeltests/get_latest/ be removed?

Re the warnings: I just get those when running 'git apply'. I didn't get them with the latest patch though, so it's fine on that front. Thanks!

Changed 5 years ago by Adrien Lemaire

Attachment: t17813.4.diff added

comment:17 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: unset

Fixed.

  • modeltests/get_latest has to be removed yes, forgot to add it in the patch.
  • Tests were broken because one will set Article._meta.get_latest_by = None, and the next test will expect it to be something. fixed by adding a tearDown that adds back the get_latest_by to the model if it's None.

comment:18 Changed 4 years ago by anonymous

Owner: set to roland

comment:19 Changed 4 years ago by paper82

Owner: changed from roland to paper82
Status: newassigned

comment:20 Changed 4 years ago by paper82

Owner: paper82 deleted
Status: assignednew

comment:21 Changed 4 years ago by paper82

Hello, new contributor here - I'd like to ask about the current state of this ticket/patch; if there is something to be done, I'd like to take a shot at it.

comment:22 Changed 4 years ago by paper82

Cc: paper82 added

comment:23 Changed 4 years ago by Nick Sandford

Triage Stage: AcceptedReady for checkin

Cleaned up patch a bit. It looks good, tests pass. Pull request here: https://github.com/django/django/pull/643.

comment:24 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

Owner: set to Anssi Kääriäinen <akaariai@…>
Resolution: fixed
Status: newclosed

In fe54377dae1357a7f102d72614a13f0ef8b2dbdf:

Fixed #17813 -- Added a .earliest() method to QuerySet

Thanks a lot to everybody participating in developing this feature.
The patch was developed by multiple people, at least Trac aliases
tonnzor, jimmysong, Fandekasp and slurms.

Stylistic changes added by committer.

comment:25 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

In f96c86b02943009d4c2e01d8e4457db040723a25:

Added missing versionadded 1.6 to docs of earliest()

Refs #17813

comment:26 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

In 7aa538357c8d94df3d5811706bf6dfe5d21421ca:

Cleaned up testing models.py added for earliest()

The main cleanup was removal of non-necessary unicode method. The
tests didn't break on py3 as the string representation was never used
in the tests.

Refs #17813. Thanks to Simon Charette for spotting this issue.

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