-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add PEP 519 support #6788
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
Add PEP 519 support #6788
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
import inspect | ||
import warnings | ||
from contextlib import contextmanager | ||
import shutil | ||
import tempfile | ||
|
||
import matplotlib | ||
from matplotlib.cbook import is_string_like, iterable | ||
|
@@ -164,3 +166,21 @@ def setup(): | |
rcdefaults() # Start with all defaults | ||
|
||
set_font_settings_for_testing() | ||
|
||
|
||
@contextmanager | ||
def closed_tempfile(suffix='', text=None): | ||
""" | ||
Context manager which yields the path to a closed temporary file with the | ||
suffix `suffix`. The file will be deleted on exiting the context. An | ||
additional argument `text` can be provided to have the file contain `text`. | ||
""" | ||
with tempfile.NamedTemporaryFile( | ||
'w+t', suffix=suffix, delete=False | ||
) as test_file: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That parenthesis here is weird. I am surprised pep8 allows this. Is there anyway to break the line in another way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with tempfile.NamedTemporaryFile('w+t', suffix=suffix,
delete=False) as test_file: fits within 79 chars, and looks reasonable, I think. |
||
file_name = test_file.name | ||
if text is not None: | ||
test_file.write(text) | ||
test_file.flush() | ||
yield file_name | ||
shutil.rmtree(file_name, ignore_errors=True) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ | |
from matplotlib import pyplot as plt | ||
from matplotlib.testing.decorators import (image_comparison, knownfailureif, | ||
cleanup) | ||
from matplotlib.testing import skip | ||
from matplotlib.testing import skip, closed_tempfile | ||
|
||
if 'TRAVIS' not in os.environ: | ||
@image_comparison(baseline_images=['pdf_use14corefonts'], | ||
|
@@ -57,14 +57,12 @@ def test_multipage_pagecount(): | |
@cleanup | ||
def test_multipage_keep_empty(): | ||
from matplotlib.backends.backend_pdf import PdfPages | ||
from tempfile import NamedTemporaryFile | ||
# test empty pdf files | ||
# test that an empty pdf is left behind with keep_empty=True (default) | ||
with NamedTemporaryFile(delete=False) as tmp: | ||
with closed_tempfile(".pdf") as tmp: | ||
with PdfPages(tmp) as pdf: | ||
filename = pdf._file.fh.name | ||
assert os.path.exists(filename) | ||
os.remove(filename) | ||
# test if an empty pdf is deleting itself afterwards with keep_empty=False | ||
with PdfPages(filename, keep_empty=False) as pdf: | ||
pass | ||
|
@@ -74,19 +72,17 @@ def test_multipage_keep_empty(): | |
ax = fig.add_subplot(111) | ||
ax.plot([1, 2, 3]) | ||
# test that a non-empty pdf is left behind with keep_empty=True (default) | ||
with NamedTemporaryFile(delete=False) as tmp: | ||
with closed_tempfile(".pdf") as tmp: | ||
with PdfPages(tmp) as pdf: | ||
filename = pdf._file.fh.name | ||
pdf.savefig() | ||
assert os.path.exists(filename) | ||
os.remove(filename) | ||
# test that a non-empty pdf is left behind with keep_empty=False | ||
with NamedTemporaryFile(delete=False) as tmp: | ||
with closed_tempfile(".pdf") as tmp: | ||
with PdfPages(tmp, keep_empty=False) as pdf: | ||
filename = pdf._file.fh.name | ||
pdf.savefig() | ||
assert os.path.exists(filename) | ||
os.remove(filename) | ||
|
||
|
||
@cleanup | ||
|
@@ -137,20 +133,17 @@ def test_grayscale_alpha(): | |
|
||
@cleanup | ||
def test_pdfpages_accept_pep_519(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those two tests look like they could be refactored into one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test_pdfpages_accept_pep_519 and test_savefig_accept_pathlib? I'm not sure it's possible to skip part of a test and have that appear in nose/pytest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmh… good question. |
||
from tempfile import NamedTemporaryFile | ||
|
||
class FakeFSPathClass(object): | ||
def __init__(self, path): | ||
self._path = path | ||
|
||
def __fspath__(self): | ||
return self._path | ||
tmpfile = NamedTemporaryFile(suffix='.pdf') | ||
tmpfile.close() | ||
with PdfPages(FakeFSPathClass(tmpfile.name)) as pdf: | ||
fig, ax = plt.subplots() | ||
ax.plot([1, 2], [3, 4]) | ||
pdf.savefig(fig) | ||
with closed_tempfile(suffix='.pdf') as fname: | ||
with PdfPages(FakeFSPathClass(fname)) as pdf: | ||
fig, ax = plt.subplots() | ||
ax.plot([1, 2], [3, 4]) | ||
pdf.savefig(fig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ... but this doesn't: the NamedTemporaryFile still has the file open and savefig writes to it. The other test closes the file and reuses only the name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in pypandoc I use this:
|
||
|
||
|
||
@cleanup | ||
|
@@ -159,13 +152,11 @@ def test_savefig_accept_pathlib(): | |
from pathlib import Path | ||
except ImportError: | ||
skip("pathlib not installed") | ||
from tempfile import NamedTemporaryFile | ||
|
||
fig, ax = plt.subplots() | ||
C3FE ax.plot([1, 2], [3, 4]) | ||
tmpfile = NamedTemporaryFile(suffix='.pdf') | ||
tmpfile.close() | ||
with PdfPages(Path(tmpfile.name)) as pdf: | ||
fig, ax = plt.subplots() | ||
ax.plot([1, 2], [3, 4]) | ||
pdf.savefig(fig) | ||
with closed_tempfile(suffix='.pdf') as fname: | ||
with PdfPages(Path(fname)) as pdf: | ||
fig, ax = plt.subplots() | ||
ax.plot([1, 2], [3, 4]) | ||
pdf.savefig(fig) |
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.
Can you please rewrite this docstring with the numpydoc format? I find the docstring relatively unclear on what exactly the function does. If I understand correctly, it creates, closes and returns a path to a file and deletes it on exiting the context. It is also unclear where the file is created. A note on how it relates and differs from
tempfile.NamedTemporaryFile
would help clarify what the function does.Also, this is publicly available - do we want this publicly available? (if not, then the docstring comment can be ignored.)
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.
Does publicly available mean should be used outside of matplotlib, or is the recommended option for other parts of matplotlib? I don't see much use outside matplotlib, unless you were already using matplotlib's testing utils.