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

ENH: Add page_number property #1856

Merged
merged 3 commits into from
Jun 19, 2023
Merged

ENH: Add page_number property #1856

merged 3 commits into from
Jun 19, 2023

Conversation

pubpub-zz
Copy link
Collaborator

add a new property to ease access to page number on all objects

is used in further fix

add a new property to ease access to page number on all objects
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (54e027a) 93.53% compared to head (33668cf) 93.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1856      +/-   ##
==========================================
+ Coverage   93.53%   93.56%   +0.02%     
==========================================
  Files          34       34              
  Lines        6625     6634       +9     
  Branches     1296     1297       +1     
==========================================
+ Hits         6197     6207      +10     
  Misses        281      281              
+ Partials      147      146       -1     
Impacted Files Coverage Δ
pypdf/_page.py 92.04% <100.00%> (+0.10%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MartinThoma MartinThoma changed the title ENH: add PageNumber property ENH: Add page_number property Jun 11, 2023
Copy link
Member

@MartinThoma MartinThoma left a comment

Choose a reason for hiding this comment

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

Only the "not attached" case is something I would like to do different. The rest looks good :-)

@pubpub-zz
Copy link
Collaborator Author

Only the "not attached" case is something I would like to do different. The rest looks good :-)

_get_page_number_by_indirect returns -1 if the page is not found(includes not attached). we will have to change it also?

@MartinThoma
Copy link
Member

_get_page_number_by_indirect returns -1 if the page is not found(includes not attached). we will have to change it also?

Oh, interesting. So we have that behavior for a long time in the code base.

I don't care too much about the internal/private functions.
However, get_page_number and get_destination_page_number already have this behavior.

I'm torn now: One the one hand, I think "-1" might cause issues that "None" or an exception would not cause.
On the other hand, inconsistencies are confusing and also lead to issues. And I want to avoid breaking changes.

Let me think about it until Sunday 🤔

@MartinThoma
Copy link
Member

(I tend to say "leave things as you already did" though - if we have want to change that, we can do it in one go for the whole codebase)

@MartinThoma MartinThoma merged commit 8938d98 into py-pdf:main Jun 19, 2023
12 checks passed
MartinThoma added a commit that referenced this pull request Jun 23, 2023
New Features (ENH):
-  Add page_number property (#1856)

Bug Fixes (BUG):
- File expansion when updating with Page Contents (#1906)
- Missing Alternate in indexed/ICCbased colorspaces (#1896)

[Full Changelog](3.10.0...3.11.0)
@mara004
Copy link

mara004 commented Jun 24, 2023

I'd encourage you to switch from -1 to None for the "no page number available" case, as reverse indexing accidents are just too likely. In my experience, many callers aren't careful about correct error handling, and None is safer in any case.

@MartinThoma
Copy link
Member

I've opened #1914 so that I don't forget about it. I want to keep pypdf stable for this year, but we had already a couple of breaking changes. I will collect them and do them all in one go.

@pubpub-zz pubpub-zz deleted the pg_numb branch September 2, 2023 09:49
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

3 participants