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

Fix override_settings compat #5668

Merged

Conversation

carltongibson
Copy link
Collaborator

Closes #2466 Ref #5658

As per #2466, the override_settings integration was updating the APISettings object correctly but was not functioning as expected since the documented usage from rest_framework.settings import api_settings is bound at import time.

This PR adds a (failing) test checking compat and adjusts the code to refresh APISettings in place, rather than replace the instance. override_settings then works as expected.

Fix was suggested by @daggaz — thanks for that! (PRs always welcome! 🙂)

@carltongibson carltongibson added this to the v3.7.4 milestone Dec 14, 2017
@carltongibson
Copy link
Collaborator Author

carltongibson commented Dec 14, 2017

@rpkilby If you have a moment, can I ask you to look at this quickly? There's just a small query over the Travis build for the first commit.

It correctly fails, but only for the Python 2 builds. It should have failed for all except lint, readme, docs.

Running tox locally on 3f9a54f I get the expected result:

ERROR:   py27-django110: commands failed
ERROR:   py34-django110: commands failed
ERROR:   py35-django110: commands failed
ERROR:   py27-django111: commands failed
ERROR:   py34-django111: commands failed
ERROR:   py35-django111: commands failed
ERROR:   py36-django111: commands failed
ERROR:   py34-django20: commands failed
ERROR:   py35-django20: commands failed
ERROR:   py36-django20: commands failed
ERROR:   py35-djangomaster: commands failed
ERROR:   py36-djangomaster: commands failed
  lint: commands succeeded
  docs: commands succeeded
  readme: commands succeeded

As such, I'm happy, but I'm just "What happened to Travis?".

I'm going to merge this as is, since I want to finish #5658. I don't think there is a problem; if there is we can come back to it; I'd be grateful though if you could double check.

Ta. 🙂

@carltongibson carltongibson merged commit 4a200d5 into encode:master Dec 14, 2017
@carltongibson carltongibson deleted the 374/fix-override_settings-compat branch March 3, 2019 19:19
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Add test checking override_settings compat

* Refresh APISettings, rather than replace

Fix suggested by @daggaz encode#2466 (comment)
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.

1 participant