| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Support context manager protocol by :class:`contextvars.Token`. Patch by | ||
| Andrew Svetlov. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Back | FazBrowse Home | New Git 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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityMaybe more test with a re-entrant context? as well as a test when an exception is raised in the context body?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThanks for the review, I'll add tests.
I don't expect any problems though since the implementation is really trivial.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityUp to you though! Generally, I'm not actually adding those kind of tests to test the implementation but rather to spot possible regressions if we change something. I actually don't know if we are that pedantic when testing other context managers so feel free not to burden the tests if you think it's not worth it!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityTests are cheap and easy to write,
I've added two: one for exit when an exception is raised and another for reentrancy.
Please feel free to ask for additional tests if needed.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityLooks good! Small question, but is there a necessity to check what happens if we call c.reset() inside the context manager? or multiple c.set() as well?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityMultiple c.set() doesn't affect the context manager, the value will be reset on exit.
c.reset() is safe if the other token was used; resetting with the same token raises RuntimeErorr as in the following example:
with c.set(1) as token: c.reset(token)Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityA couple additional tests were added to demonstrate mentioned scenarios.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.