Skip to content

Add pep 519 #8481

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

Closed
wants to merge 3 commits into from
Closed

Add pep 519 #8481

wants to merge 3 commits into from

Conversation

tacaswell
Copy link
Member

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

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/whats_new.rst if major new feature
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Apr 14, 2017
@tacaswell tacaswell mentioned this pull request Apr 14, 2017
@story645 story645 mentioned this pull request Apr 14, 2017
@anntzer
Copy link
Contributor

anntzer commented Apr 14, 2017

Only quickly skimmed through this, so I may have missed something.
What is the rationale for fspath_no_except, rather than just using (a backport of) fspath?
Of course that'll mean TypeErrors on invalid inputs, but weren't errors going to be raised just after anyways?

@tacaswell
Copy link
Member Author

I am not sure, I just rebased @aragilar PR ...

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the conversion should almost certainly occur higher, around line 554.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that looks wrong, I don't remember why I did this...

@aragilar
Copy link
Contributor

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.

@tacaswell
Copy link
Member Author

That makes sense. This is short enough I would rather vendor this than pick up another tiny dependency.

@@ -349,6 +350,7 @@ def _run(self):
output = sys.stdout
else:
output = subprocess.PIPE
command = [fspath_no_except(cmd) for cmd in command]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably just be done on self.output

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or even better to matplotlib.compat.{Popen,check_output}:

subprocess.Popen([Path("ls"), Path("-a"), Path(".")])

works (even though this is arguably silly...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by 'output', I think I meant 'outfile'..

@anntzer anntzer dismissed their stale review June 13, 2017 16:01

I still think there should be a better way than fspath_no_except, but don't block on that.

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 10, 2018
@anntzer anntzer mentioned this pull request Jan 11, 2018
6 tasks
@tacaswell tacaswell closed this Jan 21, 2018
@tacaswell tacaswell deleted the add_pep_519 branch January 21, 2018 21:12
@tacaswell
Copy link
Member Author

super-ceded by #10231

Thanks for your work on this @aragilar !

@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. status: duplicate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants