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

Adding more dedicated exceptions #739

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented Oct 2, 2024

Type of pull request

  • Bug fix (involves code and configuration changes)
  • New feature (involves code and configuration changes)
  • Documentation update
  • Something else

About

Adding 2 dedicated exceptions, following the path of #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
Copy link
Collaborator

k00ni commented Oct 3, 2024

👍 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
Copy link
Contributor Author

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
Copy link
Collaborator

k00ni commented Oct 17, 2024

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 k00ni added the stale needs decision label Oct 24, 2024
@k00ni k00ni marked this pull request as draft October 24, 2024 11:16
@k00ni
Copy link
Collaborator

k00ni commented Oct 28, 2024

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

@k00ni k00ni closed this Oct 28, 2024
@k00ni k00ni removed the stale needs decision label Oct 28, 2024
@ThomasLandauer
Copy link
Contributor Author

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
Copy link
Collaborator

k00ni commented Oct 29, 2024

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

@k00ni k00ni reopened this Oct 29, 2024
@ThomasLandauer
Copy link
Contributor Author

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

@k00ni k00ni marked this pull request as ready for review October 31, 2024 08:28
Copy link
Collaborator

@k00ni k00ni left a comment

Choose a reason for hiding this comment

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

Thanks for checking.

Here is a brief summary of my latest changes:

  • added a short comment for each new exception
  • added another exception (NotImplementedException)
  • fixed coding style issues (ran dev-tools/vendor/bin/php-cs-fixer fix locally)

@k00ni k00ni merged commit afa8deb into smalot:master Oct 31, 2024
32 checks passed
@k00ni k00ni mentioned this pull request Oct 31, 2024
1 task
@ThomasLandauer ThomasLandauer deleted the dedicated-exceptions branch October 31, 2024 09:44
@ThomasLandauer ThomasLandauer mentioned this pull request Oct 31, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants