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

Doc'd Model.MultipleObjectsReturned docs and improved documentation related with models exceptions. #13194

Merged
merged 2 commits into from
Jul 22, 2020

Conversation

adamchainz
Copy link
Sponsor Member

DoesNotExist and MultipleObjectsReturned are very similar but had very different documentation prior to this change. I also added a note that get() should generally be used for lookups that the DB guarantees uniqueness on.

Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reading much more clearly, thanks.

docs/ref/models/class.txt Outdated Show resolved Hide resolved
docs/ref/models/class.txt Outdated Show resolved Hide resolved
docs/ref/models/class.txt Outdated Show resolved Hide resolved
docs/ref/models/querysets.txt Show resolved Hide resolved
docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
docs/ref/models/querysets.txt Outdated Show resolved Hide resolved
@adamchainz adamchainz force-pushed the document_multiple_objects_returned branch from 975080a to 3d88663 Compare July 16, 2020 15:46
@adamchainz
Copy link
Sponsor Member Author

Thank you very much for all your review today 🤘

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamchainz Thanks 👍 I moved DoesNotExist to a separate commit and pushed small edits.


>>> user.supervisor_of
Traceback (most recent call last):
...
DoesNotExist: User matching query does not exist.
RelatedObjectDoesNotExist: User matching query does not exist.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RelatedObjectDoesNotExist: User matching query does not exist.
RelatedObjectDoesNotExist: User has no supervisor_of.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's good to show the right error message 😅

@@ -1844,34 +1844,42 @@ they query the database each time they're called.
.. method:: get(**kwargs)

Returns the object matching the given lookup parameters, which should be in
the format described in `Field lookups`_.
the format described in `Field lookups`_. Normally you should use lookups
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chopped Normally.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are use cases where the database doesn't have any uniqueness constraints but application level logic is relied on for unique results with given lookups. This even aplpies when using order_with_respect_to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we will chop also "... by the database". This should cover all cases.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable and shorter. I've pushed the change.

e = Entry.objects.get(id=3)
b = Blog.objects.get(id=1)
blog = Blog.objects.get(id=1)
entry = Entry.objects.get(blog=blog, id=3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
entry = Entry.objects.get(blog=blog, id=3)
entry = Entry.objects.get(blog=blog, entry_number=1)

@felixxm felixxm force-pushed the document_multiple_objects_returned branch 2 times, most recently from abdfb7d to ded6354 Compare July 22, 2020 09:56
@felixxm felixxm changed the title Improved documentation of QuerySet.get() and MultipleObjectsReturned Doc'd Model.MultipleObjectsReturned docs and improved documentation related with models exceptions. Jul 22, 2020
@felixxm felixxm force-pushed the document_multiple_objects_returned branch from ded6354 to df62816 Compare July 22, 2020 10:57
@adamchainz
Copy link
Sponsor Member Author

Changes LGTM

@adamchainz adamchainz force-pushed the document_multiple_objects_returned branch from df62816 to bc4fea9 Compare July 22, 2020 13:21
@felixxm felixxm merged commit bc4fea9 into django:master Jul 22, 2020
@adamchainz adamchainz deleted the document_multiple_objects_returned branch July 24, 2020 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants