Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ENH: Add page label support to PdfWriter #1558
ENH: Add page label support to PdfWriter #1558
Changes from 6 commits
b110225
c5fa998
39c6423
4a48c29
474b211
9c97450
55ed6c8
dbd6bce
717c882
e298494
6977af6
251390f
d56a63a
64beecb
739d117
b24b2ce
75fa103
8c0a82e
679b8f8
cd0b81e
c1c1357
ef2a6e8
daad866
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What do you think about page ranges indexes.
Now
set_page_label
requires page numbers starting with 1 while_set_page_label
requires them starting with 0 (the parameters name change accordingly frompage_number
topage_index
)Also in both cases extremes are included.
I did it this way in the public interface because I think it is more natural this way for the user that probably is setting these watching a pdf (where page numbers start from 1), but I understand if you don't agree, especially considering that the private interface is 0 based.
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.
I'd prefer to use
page_index
everywhere and hence starting from 0.Don't forget that pypdf users are Python developers. Starting a list of pages with index 0 is a lot more natural than starting with 1.
My suggestion would be to rename the parameters to
page_index_from
andpage_index_to
and letting it start with 0.@pubpub-zz / @MasterOdin What's your opinion?
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.
Agree with your approach
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.
hehehe 1 based indexing is never the answer. The more I think about it the more I agree with you. I'll wait a bit for MasterOdin answer and then I'll change that. Unfortunately, I will have to change all the indexes in the test but I knew what I was risking when I did it:sweat_smile:
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.
I would agree with making it 0-based indexed for the simple reason that all other functions that refer to pages uses a 0-based index.
I agree with @MartinThoma that using
page_index
is the more "proper" way to refer to this as people may conflatepage_number
with a 1-based index scheme, whereaspage_index
, in my opinion, really only refers to 0-based index schemes in the context of Python.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.
Thank you! I'll do that
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.
The last commit changes everything to 0-based indexing. Are you convinced by including both extremes or should we do lower included and upper excluded?