Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
setattr context manager.#10314
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
setattr context manager.#10314
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Could this be done with |
Indeed, once we switch to Py3-only (or if we decide to add mock as a dependency) it'll be as simple as replacing cbook._setattr_cm by unittest.mock.patch.multiple... |
I'd still be 👍 to merge this. It improves things, reduces the lines of code. I'm hesitant to wait for releases that are 6 months out. |
lib/matplotlib/font_manager.py Outdated
| with open(fpath, 'rb') as fh: | ||
| try: | ||
| font = afm.AFM(fh) | ||
| except RuntimeError: |
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.
Couldn't you simplify this by leaving out the inner try and catching RuntimeError in the outer try?
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 guess open() cannot raise RuntimeError, so yes...
| @@ -146,11 +146,6 @@ def context(style, after_reset=False): | |||
| mpl.rcdefaults() | |||
| try: | |||
| use(style) | |||
| except: | |||
| # Restore original settings before raising errors during the update. | |||
| mpl.rcParams.update(initial_settings) | |||
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.
Why is this removed?
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.
because it's already restored in the finally: just below...
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.
Ok, learned something new today: The finally is executed before the raise. Then, it‘s ok.
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.
even
try: return 1finally: return 2will return 2...
https://docs.python.org/3/reference/compound_stmts.html#the-try-statement
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.
Yeah, thefinally is executed no matter what else happens.
dbd5db6 to34c7ce4Compare619460d to3fa657aCompareLooks like this will be in MPL3.0 Did you want to make the change suggested above? |
Actually unittest.mock.patch.multiple does not work for previously non-existing attributes (where a delattr has to be done at the end), which we use for mpl_image_comparison_parameters. It is also nearly twice slower. |
This context manager was added to master in690b213 viamatplotlib#10314. We do not wantto backport that entire commit, however the backport ofmatplotlib#11407requires this context manger.It is private and self contained to low-risk to backport
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
The setattr-contextmanager that was proposed in#10292 (comment). Just quickly grepped for all the finallys in the codebase.
I also thought about doing something similar for set_prop() / get_prop() but that only appears twice throughout the codebase afaict so it's not really worth it.
PR Checklist