Code

Opened 2 years ago

Closed 15 months ago

Last modified 15 months 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 2 years ago.
first_latest_with_tests.patch (4.2 KB) - added by jimmysong 2 years ago.
added tests, fixed the fact that the model manager didn't have a first() method.
t17813.diff (17.0 KB) - added by Fandekasp 2 years ago.
Added new get_first_by Objects field. More tests. Doc updated
t17813.2.diff (17.2 KB) - added by Fandekasp 2 years ago.
new diff with first renamed into earliest
t17813.3.diff (14.7 KB) - added by Fandekasp 2 years ago.
get_earliest_by attr related code reverted
t17813.3.2.diff (12.4 KB) - added by Fandekasp 2 years ago.
t17813.4.diff (16.8 KB) - added by Fandekasp 2 years ago.

Download all attachments as: .zip

Change History (33)

Changed 2 years ago by tonnzor

comment:1 Changed 2 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

What's the use case for this method?

comment:2 Changed 2 years ago by russellm

  • Easy pickings set
  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted

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 2 years ago by jimmysong

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

comment:3 Changed 2 years ago by jimmysong

  • Needs tests unset
  • Resolution set to fixed
  • Status changed from new to closed

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 2 years ago by jimmysong

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:5 Changed 2 years ago by Fandekasp

  • Cc lemaire.adrien@… added
  • Owner changed from nobody to Fandekasp
  • Status changed from reopened to new

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 2 years ago by Fandekasp

Added new get_first_by Objects field. More tests. Doc updated

comment:6 Changed 2 years ago by Fandekasp

  • 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 2 years ago by hcarvalhoalves

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 2 years ago by Fandekasp

  • 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 2 years ago by Fandekasp

new diff with first renamed into earliest

comment:9 Changed 2 years ago by Fandekasp

  • Patch needs improvement unset

Done.

comment:10 Changed 2 years ago by hcarvalhoalves

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 2 years ago by Fandekasp

  • Owner Fandekasp deleted

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

comment:12 Changed 2 years ago by julien

  • 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 2 years ago by Fandekasp

get_earliest_by attr related code reverted

comment:13 Changed 2 years ago by Fandekasp

  • Patch needs improvement unset

done

comment:14 Changed 2 years ago by julien

  • 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 2 years ago by Fandekasp

comment:15 Changed 2 years ago by Fandekasp

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 2 years ago by Fandekasp (previous) (diff)

comment:16 Changed 2 years ago by julien

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 2 years ago by Fandekasp

comment:17 Changed 2 years ago by Fandekasp

  • 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 17 months ago by anonymous

  • Owner set to roland

comment:19 Changed 17 months ago by paper82

  • Owner changed from roland to paper82
  • Status changed from new to assigned

comment:20 Changed 17 months ago by paper82

  • Owner paper82 deleted
  • Status changed from assigned to new

comment:21 Changed 17 months 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 17 months ago by paper82

  • Cc paper82 added

comment:23 Changed 15 months ago by slurms

  • Triage Stage changed from Accepted to Ready 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 15 months ago by Anssi Kääriäinen <akaariai@…>

  • Owner set to Anssi Kääriäinen <akaariai@…>
  • Resolution set to fixed
  • Status changed from new to closed

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 15 months ago by Anssi Kääriäinen <akaariai@…>

In f96c86b02943009d4c2e01d8e4457db040723a25:

Added missing versionadded 1.6 to docs of earliest()

Refs #17813

comment:26 Changed 15 months 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.