-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Fix JSON format bug and duplicate test clean-up #2418
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
base: master
Are you sure you want to change the base?
Conversation
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.
Overall these changes seem good but there are a number of deletions that seem arbitrary. I'd be happy to merge this in once the notes I left are addressed.
| from django.test import TestCase | ||
| from django.urls import reverse | ||
|
|
||
|
|
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.
Two blank lines before a top level function is preferred to adhere to pep8:
| def test_post(self): | ||
| """ | ||
| Test that a response is returned. | ||
| """ |
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.
Could the docstrings in these functions have been improved instead of removed?
| response = self.client.post( | ||
| self.api_url, | ||
| data=json.dumps({ | ||
| 'text': u'سلام' |
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 had to look this one up, but this seems safe to remove ✅
(Python 3 no longer needs the u bytemark, good to know)
|
|
||
| def test_delete(self): | ||
| response = self.client.delete(self.api_url) | ||
|
|
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.
A space between test calls and assertions is preferred.
This pull request fixes a small bug and improves the structure of the test cases.
Fixed:
jsonmodule as a format parameterClean-up:
Let me know if anything else is needed. Happy to help!