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

Pdfpages pagecount convenience getter method #2440

Merged
merged 2 commits into from Sep 19, 2013

Conversation

megies
Copy link
Contributor

@megies megies commented Sep 19, 2013

Add convenience method get_pagecount() to PdfPages that returns the current number of pages in the multipage file. Also adds a simple test for it.

@WeatherGod
Copy link
Member

Seems simple enough and has a test to boot! Waiting for Travis results, but this gets a +1 from me

@megies
Copy link
Contributor Author

megies commented Sep 19, 2013

I am not very familiar with the test setup here, but I assumed the @cleanup decorator is all that is needed here..

@WeatherGod
Copy link
Member

Yes, that is all that was needed. Merging!

WeatherGod added a commit that referenced this pull request Sep 19, 2013
Pdfpages pagecount convenience getter method
@WeatherGod WeatherGod merged commit fc94ddb into matplotlib:v1.3.x Sep 19, 2013
@WeatherGod
Copy link
Member

Gah! How do I undo what I just did? This PR was set against the maintenance branch and not master.

@megies
Copy link
Contributor Author

megies commented Sep 19, 2013

No way to undo it.
But my last PRs I always set against v1.3.x (and I never got a complaint). After merging them into v1.3.x, v1.3.x was merged into master subsequently.

@WeatherGod
Copy link
Member

bugfixes go into maintenance branch (v1.3.x), new features go into master. This was a (tiny) new feature, not a bugfix.

@megies
Copy link
Contributor Author

megies commented Sep 19, 2013

Ok, sorry. My bad then, didn't know.
Although, my last PR (#2416) also was more of "new feature" than "fix" and I wasn't prompted to set it against master. Anyway, I'll stick to this rule from now on.
If it really should not be in here, the only solution is a revert commit I think.

@pelson
Copy link
Member

pelson commented Sep 20, 2013

Again, gladly this is an innocuous enhancement, rather than a deep algorithmic change, and if this doesn't get reverted, I wouldn't mind.

Obviously the git paradigm of always moving forward with commits means that to revert a commit is literally to apply an inverse changeset - the original commit will always be there unless we break the golden rule of removing commits (which we really don't want to do - that would be a major headache).

@mdboom
Copy link
Member

mdboom commented Sep 20, 2013

Yeah -- I'm fine with this staying on the branch, though strictly not a bugfix.

@megies
Copy link
Contributor Author

megies commented Sep 20, 2013

the original commit will always be there unless

Yes, no way to get rid of the commit itself but the change could be undone. In any case, seems its settled and its not a big deal anyway.
I'll take care to set PRs against the correct branch in the future, thanks for sorting it out.

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