-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
b110225
ENH: Add page label support to PdfWriter
lorenzomanini c5fa998
Merge branch 'main' of https://github.com/py-pdf/pypdf
lorenzomanini 39c6423
fix import, fix overlapping ranges behaviour
lorenzomanini 4a48c29
add tests
lorenzomanini 474b211
Merge remote-tracking branch 'upstream/main'
lorenzomanini 9c97450
Merge branch 'main' of https://github.com/lorenzomanini/pypdf
lorenzomanini 55ed6c8
Apply suggestions from code review: fix mypy
lorenzomanini dbd6bce
Apply suggestions from code review: fix mypy
lorenzomanini 717c882
Fix mypy
lorenzomanini e298494
fix flake
lorenzomanini 6977af6
Moved nums functions to _page_labels.py
lorenzomanini 251390f
Added tests
lorenzomanini d56a63a
Added Docs
lorenzomanini 64beecb
add PageLabelStyle constants
lorenzomanini 739d117
More explicit PageLabelStyle constants
lorenzomanini b24b2ce
Apply suggestions from code review: fix Doc
lorenzomanini 75fa103
fix flake8
lorenzomanini 8c0a82e
add lorenzomanini to CONTRIBUTORS.md
lorenzomanini 679b8f8
Merge branch 'main' into main
lorenzomanini cd0b81e
change set_page_label page indexing to 0 based
lorenzomanini c1c1357
Apply suggestions from code review: fix Doc
lorenzomanini ef2a6e8
Merge remote-tracking branch 'upstream/main'
lorenzomanini daad866
fix test
lorenzomanini File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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?