Skip to content

Change remaining HttpResponse to JsonResponse #989

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

Merged
merged 3 commits into from
Jul 2, 2021
Merged

Conversation

Andrew-Chen-Wang
Copy link
Contributor

Fixes #987

Description of the Change

Changes any valid HttpResponse(json.dumps(), etc... to django.http.JsonResponse

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

* Add Andrew-Chen-Wang to AUTHORS
@codecov
Copy link

codecov bot commented Jun 19, 2021

Codecov Report

Merging #989 (5af4efc) into master (d90bb34) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
- Coverage   96.62%   96.55%   -0.07%     
==========================================
  Files          31       31              
  Lines        1716     1715       -1     
==========================================
- Hits         1658     1656       -2     
- Misses         58       59       +1     
Impacted Files Coverage Δ
oauth2_provider/models.py 98.67% <ø> (ø)
oauth2_provider/views/introspect.py 100.00% <100.00%> (ø)
oauth2_provider/oauth2_validators.py 93.42% <0.00%> (-0.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d90bb34...5af4efc. Read the comment docs.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

shouldn't the unit tests updated?

@Andrew-Chen-Wang
Copy link
Contributor Author

@auvipy not sure what this means since I went straight from master and did not change models.py...:

flake8 run-test-pre: PYTHONHASHSEED='384501872'
24
  flake8 run-test: commands[0] | flake8 /home/runner/work/django-oauth-toolkit/django-oauth-toolkit
25
  [1929] /home/runner/work/django-oauth-toolkit/django-oauth-toolkit$ /home/runner/work/django-oauth-toolkit/django-oauth-toolkit/.tox/flake8/bin/flake8 .
26
  ./oauth2_provider/models.py:566:8: BLK100 Black would make changes.
27
  ERROR: InvocationError for command /home/runner/work/django-oauth-toolkit/django-oauth-toolkit/.tox/flake8/bin/flake8 . (exited with code 1)

@Andrew-Chen-Wang
Copy link
Contributor Author

Andrew-Chen-Wang commented Jun 19, 2021

@auvipy we should pin flake8 in tox otherwise PRs could magically fail due to an unexpected upgrade:

https://github.com/jazzband/django-oauth-toolkit/blob/d90bb348f00886ae8321f8194f6275be9aa08f80/tox.ini#L75-L79

or even do a cron job of linting

@auvipy
Copy link
Contributor

auvipy commented Jul 1, 2021

isnt there any test for views?

@Andrew-Chen-Wang
Copy link
Contributor Author

@auvipy what do you mean "aren't there any tests for views?" Seems like the normal test suite is running, just the style isn't right.

Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

changelog entry needed

@Andrew-Chen-Wang
Copy link
Contributor Author

@auvipy updated

@auvipy
Copy link
Contributor

auvipy commented Jul 2, 2021

Required statuses must pass before merging

@Andrew-Chen-Wang
Copy link
Contributor Author

finally found the linting problem. thanks for approving @auvipy !

@Andrew-Chen-Wang Andrew-Chen-Wang merged commit 3716ec4 into master Jul 2, 2021
@Andrew-Chen-Wang Andrew-Chen-Wang deleted the jsonresponse branch July 2, 2021 08:27
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.

Change JSON based HttpResponse to JSONResponse
2 participants