smalot / pdfparser

PdfParser, a standalone PHP library, provides various tools to extract data from a PDF file.
GNU Lesser General Public License v3.0
2.41k stars 537 forks source link

Adding more dedicated exceptions #739

Closed ThomasLandauer closed 3 weeks ago

ThomasLandauer commented 1 month ago

Type of pull request

About

Adding 2 dedicated exceptions, following the path of https://github.com/smalot/pdfparser/pull/433

2 remarks:

Checklist for code / configuration changes

In case you changed the code/configuration, please read each of the following checkboxes as they contain valuable information:

k00ni commented 1 month ago

:+1: I like your idea of dedicated exceptions. While we are at it, can you think of other exception types?

The remarks are also important, but I would like to discuss these in a separate issue/pull request. A dedicated CONTRIBUTING.md file seems to be a good idea. Would you mind suggesting one in a separate PR?

ThomasLandauer commented 1 month ago

While we are at it, can you think of other exception types?

Fulltext search for \Exception yields some hits. Do you want me to go through them and create exceptions? (But I'd just take the current exception message and create a class from it - I won't research the context) Or just leave it up to others who have a real need (and know why/when it occurs)?

A dedicated CONTRIBUTING.md file seems to be a good idea. Would you mind suggesting one in a separate PR?_

I'm a beginner with git, so I'm probably the wrong person. As a starting point, you can copy over what you currently have. I was annoyed yesterday to find your checklist only after I had finished my PR - so the idea is just to tell people upfront :-)

k00ni commented 1 month ago

Sorry for my delayed response. To move this forward, the code should be inspected to find further instances of these exceptions. Only replacing a few instances without covering the whole code base might lead to false implications by developers when using the API.

@ThomasLandauer would you mind searching for further instances where using these new exceptions might be a good idea?

k00ni commented 3 weeks ago

Closing this for now due to inactivity by the author. Feel free to comment when you want to continue working on it.

ThomasLandauer commented 3 weeks ago

Sorry, I didn't know what to do exactly. Do you want me to just replace all occurrences of "my" 2 exceptions? Or create a new exception for every other \Exception in the code base too?

k00ni commented 3 weeks ago

Please go through the code to find further instances of these exceptions and replace them with the new ones when appropriate.

ThomasLandauer commented 3 weeks ago

There are ~20 places where exceptions are thrown, but these 2 new appear only once. So IMO you can merge this.