Skip to content
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

Multipage pdf with statement #2416

Merged
merged 4 commits into from Sep 18, 2013

Conversation

megies
Copy link
Contributor

@megies megies commented Sep 12, 2013

This makes it possible to use the with statement with multipage pdf, so that it gets closed automatically.

@mdboom
Copy link
Member

mdboom commented Sep 12, 2013

👍

Or using the with statement::

with PdfPages('foo.pdf') as pp:
fig.savefig(pp, format='pdf') # note the format argument!
Copy link
Member

Choose a reason for hiding this comment

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

If there are two ways to do the same thing, and one of them has an obvious gotcha then lets scrap that way!
I think you should change the variable pp to pdf and just use that to save the fig rather than showing two possible ways...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally with you, just reluctant to override old stuff I guess (and I still remember having to from __future__ import with_statement at top of code..)
I'll change it to only show the "with" statement usage.

Copy link
Member

Choose a reason for hiding this comment

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

You're not stuck on Python 2.6 are you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we just had to support Python 2.5 for a rather long time (were one needed to import it from __future__) for some project a few years ago..

Copy link
Member

Choose a reason for hiding this comment

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

Ah - thank goodness for dropping 2.5 😄.

We should definitely just have the one preferred approach in this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

..already commited to change this. (see 303477a, c60dfdd)

mdboom added a commit that referenced this pull request Sep 18, 2013
@mdboom mdboom merged commit eae31b1 into matplotlib:v1.3.x Sep 18, 2013
@megies megies deleted the multipage_pdf_with_statement branch September 18, 2013 14:34
@megies megies restored the multipage_pdf_with_statement branch September 18, 2013 14:34
@WeatherGod
Copy link
Member

This new feature got added into the maintenance branch. Flagging this PR for possible reversion out of 1.3.x. This should be done prior to releasing 1.3.1.

@pelson
Copy link
Member

pelson commented Sep 20, 2013

This new feature got added into the maintenance branch.

Good spot. I agree that new features shouldn't have been merged into v1.3.x, but since this is such a minor and obvious change, I'm also not particularly bothered if it doesn't get reverted in this case. I leave it to the discretion of @mdboom.

@mdboom
Copy link
Member

mdboom commented Sep 20, 2013

Yeah -- I think we'll just leave this on the maintenance branch. In general, though, that should just be for bugfixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants