Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#17813 closed New feature (fixed)

Implement an opposite method for Entry.objects.latest

Reported by: Artem Skoretskiy Owned by: Anssi Kääriäinen <akaariai@…>
Component: Database layer (models, ORM) Version: dev
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 Artem Skoretskiy 12 years ago.
first_latest_with_tests.patch (4.2 KB ) - added by jimmysong 12 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 12 years ago.
Added new get_first_by Objects field. More tests. Doc updated
t17813.2.diff (17.2 KB ) - added by Adrien Lemaire 12 years ago.
new diff with first renamed into earliest
t17813.3.diff (14.7 KB ) - added by Adrien Lemaire 12 years ago.
get_earliest_by attr related code reverted
t17813.3.2.diff (12.4 KB ) - added by Adrien Lemaire 12 years ago.
t17813.4.diff (16.8 KB ) - added by Adrien Lemaire 12 years ago.

Download all attachments as: .zip

Change History (33)

by Artem Skoretskiy, 12 years ago

Attachment: first_latest.patch added

comment:1 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedDesign decision needed

What's the use case for this method?

comment:2 by Russell Keith-Magee, 12 years ago

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.

by jimmysong, 12 years ago

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

comment:3 by jimmysong, 12 years ago

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

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

comment:5 by Adrien Lemaire, 12 years ago

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.

by Adrien Lemaire, 12 years ago

Attachment: t17813.diff added

Added new get_first_by Objects field. More tests. Doc updated

comment:6 by Adrien Lemaire, 12 years ago

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 by Henrique C. Alves, 12 years ago

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 by Adrien Lemaire, 12 years ago

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.

by Adrien Lemaire, 12 years ago

Attachment: t17813.2.diff added

new diff with first renamed into earliest

comment:9 by Adrien Lemaire, 12 years ago

Patch needs improvement: unset

Done.

comment:10 by Henrique C. Alves, 12 years ago

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 by Adrien Lemaire, 12 years ago

Owner: Adrien Lemaire removed

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

comment:12 by Julien Phalip, 12 years ago

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.

by Adrien Lemaire, 12 years ago

Attachment: t17813.3.diff added

get_earliest_by attr related code reverted

comment:13 by Adrien Lemaire, 12 years ago

Patch needs improvement: unset

done

comment:14 by Julien Phalip, 12 years ago

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!

by Adrien Lemaire, 12 years ago

Attachment: t17813.3.2.diff added

comment:15 by Adrien Lemaire, 12 years ago

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 12 years ago by Adrien Lemaire (previous) (diff)

comment:16 by Julien Phalip, 12 years ago

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!

by Adrien Lemaire, 12 years ago

Attachment: t17813.4.diff added

comment:17 by Adrien Lemaire, 12 years ago

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 by anonymous, 11 years ago

Owner: set to roland

comment:19 by paper82, 11 years ago

Owner: changed from roland to paper82
Status: newassigned

comment:20 by paper82, 11 years ago

Owner: paper82 removed
Status: assignednew

comment:21 by paper82, 11 years ago

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 by paper82, 11 years ago

Cc: paper82 added

comment:23 by Nick Sandford, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

In f96c86b02943009d4c2e01d8e4457db040723a25:

Added missing versionadded 1.6 to docs of earliest()

Refs #17813

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

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