Reveal previously hidden AttributeErrors and TypeErrors by akx · Pull Request #3668 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This ensures AttributeErrors and TypeErrors do not get hidden when DRF is just looking for a get_queryset().
Prob worth issuing as two separate PRs.
The get_queryset ones are simple enough and would prob accept those as-is.
The .count() I'd want to give more review to.
Sure, I'll snip that one out.
| @@ -169,7 +170,7 @@ class Meta: |
|---|
| file_path_field = FilePathField(path='/tmp/') |
| """) |
| self.assertEqual(unicode_repr(TestSerializer()), expected) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing any windows test failures should probably be treated as a separate PR.
I assume that's what this change is about, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah -- though it's a bit odd that repr would act differently across platforms...
I'll split the test stuff into yet another PR. :)
| class GetQuerysetRaises(generics.ListAPIView): |
| def get_queryset(self): |
| raise CheckAttributeError("Something terrible occurred deep down in the call stack") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a regular AttributeError would be okay here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps return self.this_attribute_does_not_exist?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I subclassed AttributeError for the test for the purpose of being able to check for that exact exception being propagated up, which was the point of the patch in the first place :)
Okay, on review at this point I'd actually rather have this, but without the test cases.
It's a case of evidently improved behavior, and I think they'd actually add more noise than value in this case. Otherwise fix looks good, so happy to move once tests have been dropped. 👍
Well, if you're sure! I'll take the test cases out.
Quietly catching AttributeError and TypeError when calling
get_queryset() is rather insidious, as those exceptions get caught no
matter where they might happen in the call stack.
May seem counter-intuitive, but they come with an associated cost, and can be okay not to strictly test against obviously better/cleaner behavior.
tomchristie added a commit that referenced this pull request
Reveal previously hidden AttributeErrors and TypeErrors
This was referenced
Mar 9, 2017
2 participants
