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.
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.
I guess open() cannot raise RuntimeError, so yes...
| use(style) | ||
| except: | ||
| # Restore original settings before raising errors during the update. | ||
| mpl.rcParams.update(initial_settings) |
There was a problem hiding this comment.
because it's already restored in the finally: just below...
There was a problem hiding this comment.
Ok, learned something new today: The finally is executed before the raise. Then, it‘s ok.
There was a problem hiding this comment.
even
try: return 1
finally: return 2
will return 2...
https://docs.python.org/3/reference/compound_stmts.html#the-try-statement
There was a problem hiding this comment.
Yeah, the finally is executed no matter what else happens.
anntzer
force-pushed
the
setattr-cm
branch
4 times, most recently
from
dbd5db6 to
34c7ce4
Compare
anntzer
force-pushed
the
setattr-cm
branch
3 times, most recently
from
619460d to
3fa657a
Compare
|
Looks 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 in 690b213 via matplotlib#10314. We do not want to backport that entire commit, however the backport of matplotlib#11407 requires this context manger. It is private and self contained to low-risk to backport
5 participants
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