-
Notifications
You must be signed in to change notification settings - Fork 575
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
Deprecate the use of settings objects as context managers #1327
Comments
I tried just adding Of our tests: 1115 failed, 743 passed. Not recommended for new contributors 😭 |
I have some time for open-source this weekend, I might pick this up if nobody minds? |
If you have some time I'd love a review for my open PRs, and after (or before) that this one is all yours. |
👍
Any particular order, or oldest first?
… On 14 Jun 2018, at 21:35, Zac Hatfield-Dodds ***@***.***> wrote:
If you have some time I'd love a review for my open PRs, and after (or before) that this one is all yours.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I’m going to have a quick look at this. |
Ah, we use settings as a context manager in It’s not immediately obvious to me how we unpick it from there, but it’s been a while since I‘ve had to think about this code… |
The obvious option to me would be to add a private context manager function Do you still want to do this, or shall I? |
I’m doing it. Hopefully at least a WIP PR by the end of the day. |
This API was a bad idea and should be removed, leaving settings to be determined by global state or decorators only. Using a settings object as a context manager is somewhat ambiguous (should it happen when the test is defined, or executed? certainly not within a test...) and just plain unnecessary.
The text was updated successfully, but these errors were encountered: