Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Switch to None or Exception for page_number / get_page_number / get_destination_page_number #1914

Closed
MartinThoma opened this issue Jun 24, 2023 · 4 comments
Assignees
Labels
breaking-change A planned breaking change
Milestone

Comments

@MartinThoma
Copy link
Member

Explanation

If a page is not attached to a document, it does not have a page number. So we cannot return a "normal" number.
At the moment, we return -1.

However, callers (users of the library) might not be careful with error handling.

For this reason I suggest to change the behavior to either return None or raise an Exception. I'm not sure what is better.

Pro returning None:

  • It makes intuitive sense what it means
  • It is part of the type annotation and mypy will complain about it if you don't handle that
  • Callers just need to check for None and don't need to catch an exception - which I personally prefer.
@MartinThoma MartinThoma self-assigned this Jun 24, 2023
@MartinThoma MartinThoma added the breaking-change A planned breaking change label Jun 24, 2023
@pubpub-zz
Copy link
Collaborator

@MartinThoma
How long do you intend to let this thread open?

@MartinThoma MartinThoma added this to the pypdf==4.0.0 milestone Jul 9, 2023
@MartinThoma
Copy link
Member Author

Good question. I think I want to tackle this with the pypdf==4.0.0 release (if we do it at all), but not earlier. So it will stay open for quite a while.

I want to have that release in early January 2024.

What is your opinion on the topic @pubpub-zz ?

@pubpub-zz
Copy link
Collaborator

-1 is not an issue for me : it is consistent with functions like "find_in".find("to_be_found") in any case we need to add a test is None or <0 as 0 is a valid result.
in any case an exception is not a good option

@pubpub-zz
Copy link
Collaborator

PS: what about converting this thread to a dicussion/question

@py-pdf py-pdf locked and limited conversation to collaborators Jul 24, 2023
@MartinThoma MartinThoma converted this issue into discussion #2010 Jul 24, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
breaking-change A planned breaking change
Projects
None yet
Development

No branches or pull requests

2 participants