-
Notifications
You must be signed in to change notification settings - Fork 4
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
Validate review returns the right content type #258
Conversation
5197585
to
1955fe6
Compare
Looking ok, although, please:
|
this branch is on hold, and will be merged targetting the 1.0.1 release |
1955fe6
to
8a3c17e
Compare
You're clear to rebase (again, sorry), now that the 1.0.0 has been released |
8a3c17e
to
d156519
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need to add an item to the changelog, please.
d156519
to
cfb5d1e
Compare
Done |
@@ -242,7 +242,7 @@ def get(self, request, **kwargs): | |||
try: | |||
formidable = self.get_formidable_object(kwargs) | |||
except Formidable.DoesNotExist: | |||
raise exceptions.Http404() | |||
raise exceptions.NotFound() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think we can only return the reference of the exception here raise exceptions.NotFound
, not an instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind , just want to know why a thing is better than another :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dunno. I've always told that if you're not passing any argument to the exception's constructor, then there is no point of raising an instance, but a reference to the exception class suffice. It was a nitpick anyway. We can leave it as is 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally, I think the code is actually transforming the class to an instance automatically.
In the final exception handler, we can see something like this:
https://github.com/encode/django-rest-framework/blob/master/rest_framework/views.py#L433
But, isinstance
does not work on class itself.
In [3]: class C:
...: pass
...:
In [4]: isinstance(C(), C)
Out[4]: True
In [5]: isinstance(C, C)
Out[5]: False
So I guess, something in the code, the class is automatically instanciated.
Same thing for the default exception_handler
https://github.com/encode/django-rest-framework/blob/master/rest_framework/views.py#L58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Review
This PR closes #257