Conversation
|
Only quickly skimmed through this, so I may have missed something. |
|
I am not sure, I just rebased @aragilar PR ... |
anntzer
left a comment
anntzer
left a comment
There was a problem hiding this comment.
We should figure out whether this fspath_no_except business is actually needed.
| _png.write_png(renderer._renderer, filename_or_obj, | ||
| self.figure.dpi, metadata=metadata) | ||
| _png.write_png( | ||
| renderer._renderer, fspath_no_except(filename_or_obj), |
There was a problem hiding this comment.
the conversion should almost certainly occur higher, around line 554.
There was a problem hiding this comment.
Yeah, that looks wrong, I don't remember why I did this...
|
Sorry, this kinda dropped off my todo list... The logic behind fspath_no_except was that there were numerous places where matplotlib allowed either strings/unicode/bytes (which fspath is fine with), or actual files, which fspath would blow up on. fspath_no_except allows them to pass through with no issues. It's written in such a way that when matplotlib drops support for older python versions, the backport of fspath can just be removed without any reworking. |
|
That makes sense. This is short enough I would rather vendor this than pick up another tiny dependency. |
| output = sys.stdout | ||
| else: | ||
| output = subprocess.PIPE | ||
| command = [fspath_no_except(cmd) for cmd in command] |
There was a problem hiding this comment.
This should probably just be done on self.output
There was a problem hiding this comment.
or even better to matplotlib.compat.{Popen,check_output}:
subprocess.Popen([Path("ls"), Path("-a"), Path(".")])
works (even though this is arguably silly...)
There was a problem hiding this comment.
by 'output', I think I meant 'outfile'..
anntzer
dismissed
their stale review
I still think there should be a better way than fspath_no_except, but don't block on that.
tacaswell
modified the milestones:
2.1 (next point release),
2.2 (next next feature release)
tacaswell
added
the
Release critical
4 participants
Thank you so much for your PR! To help us review, fill out the form to the best of your ability. Also, please make use of the development guide at http://matplotlib.org/devdocs/devel/index.html
Replaces #6788 to support the Path protocol (pep 519)
PR Summary
PR Checklist