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

Adding a more explicit error message when a view does have a get_queryset method but it returned nothing #5348

Merged
merged 3 commits into from Aug 22, 2017

Conversation

fbidu
Copy link
Contributor

@fbidu fbidu commented Aug 22, 2017

Description

Hello! I have been using DRF for a while and was looking at some issues to start contributing.

I found issue #4585 and tried to solve the confusing error message when a .get_queryset() method exists but does not return anything.


Closes #4585

Copy link
Collaborator

@carltongibson carltongibson left a comment

@fbidu Thanks for this. Welcome on board! 🙂

I just think a better error message would be helpful. If we give the class then it's an easier find.

@@ -122,6 +122,9 @@ def has_permission(self, request, view):

if hasattr(view, 'get_queryset'):
queryset = view.get_queryset()
assert queryset is not None, (
'The `.get_queryset()` method from the view did not return anything.'
Copy link
Collaborator

@carltongibson carltongibson Aug 22, 2017

Choose a reason for hiding this comment

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

Can we give a more informative message here?

Perhaps:

'Return of {}.get_queryset() was None'.format(view.__class__.__name__)

Copy link
Contributor Author

@fbidu fbidu Aug 22, 2017

Choose a reason for hiding this comment

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

Thank you!

Yeah, I agree on that. Changing the message now

@fbidu
Copy link
Contributor Author

fbidu commented Aug 22, 2017

@carltongibson done 😃

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 22, 2017

@fbidu Thanks.

@carltongibson carltongibson requested a review from rpkilby Aug 22, 2017
@carltongibson
Copy link
Collaborator

carltongibson commented Aug 22, 2017

@rpkilby Do we need a test on this?

The original issue came down to the complaint that the assertion 4 lines below the change here is misleading when queryset ends up being None.

I'm half-inclined to say the extra assert is enough. (But I need a 2nd Opinion 🙂)

@carltongibson carltongibson added this to the 3.6.5 Release milestone Aug 22, 2017
@rpkilby
Copy link
Member

rpkilby commented Aug 22, 2017

I'm always in favor of adding tests, but this seems like an exception. The assertion is only raised under very obvious circumstances.

Copy link
Member

@rpkilby rpkilby left a comment

👍

@carltongibson
Copy link
Collaborator

carltongibson commented Aug 22, 2017

@fbidu Awesome work, thank you. And once again, welcome aboard! 💃🏼

@carltongibson carltongibson merged commit 08ec276 into encode:master Aug 22, 2017
1 check passed
@Neraste
Copy link

Neraste commented Aug 23, 2017

That's sweet from you!

@fbidu
Copy link
Contributor Author

fbidu commented Aug 23, 2017

@carltongibson @Neraste @rpkilby thanks!! Happy to start helping this project that did so much for me 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants