#17813 closed New feature (fixed)
Implement an opposite method for Entry.objects.latest
Reported by: | Artem Skoretskiy | Owned by: | |
---|---|---|---|
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)
Change History (33)
by , 13 years ago
Attachment: | first_latest.patch added |
---|
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 13 years ago
Easy pickings: | set |
---|---|
Needs documentation: | set |
Needs tests: | set |
Triage Stage: | Design decision needed → 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.
by , 13 years ago
Attachment: | first_latest_with_tests.patch added |
---|
added tests, fixed the fact that the model manager didn't have a first() method.
comment:3 by , 13 years ago
Needs tests: | unset |
---|---|
Resolution: | → fixed |
Status: | new → 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 by , 13 years ago
Patch needs improvement: | set |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
comment:5 by , 13 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | reopened → 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.
by , 13 years ago
Attachment: | t17813.diff added |
---|
Added new get_first_by Objects field. More tests. Doc updated
comment:6 by , 13 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 , 13 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 , 13 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.
comment:10 by , 13 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 , 13 years ago
Owner: | removed |
---|
I see your point. Let's wait for a core developer comment
comment:12 by , 13 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.
comment:14 by , 13 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 , 13 years ago
Attachment: | t17813.3.2.diff added |
---|
comment:15 by , 13 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
comment:16 by , 13 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 , 13 years ago
Attachment: | t17813.4.diff added |
---|
comment:17 by , 13 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 , 12 years ago
Owner: | set to |
---|
comment:19 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:20 by , 12 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:21 by , 12 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 , 12 years ago
Cc: | added |
---|
comment:23 by , 12 years ago
Triage Stage: | Accepted → 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 by , 12 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | new → closed |
What's the use case for this method?