mirror of
https://github.com/antonioribeiro/google2fa.git
synced 2026-04-26 16:45:49 +03:00
[GH-ISSUE #115] Base exception #523
Labels
No labels
bug
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/google2fa#523
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @poisa on GitHub (Feb 5, 2019).
Original GitHub issue: https://github.com/antonioribeiro/google2fa/issues/115
Currently you have to catch every single exception this package throws. If you make a generic exception, say
Google2faExceptionand have all other exceptions extend this one it is much easier for consumers of this package to catch anything thrown by it. Sometimes you don't care exactly what exception was thrown but you'd like to identify the general area (or package) of the problem.Would you be interested in a pull request for this?
@antonioribeiro commented on GitHub (Mar 19, 2019):
Sorry for the delay, but yes, please, would be nice.
@poisa commented on GitHub (Mar 22, 2019):
No problem. I just had a baby so I’m pretty much not sleeping (or working!). As soon as I’m back, if no one took care of this, I’ll give it a go. Should be pretty straight forward.
@poisa commented on GitHub (Jul 3, 2019):
I've looked at the code with the new changes but I think it's a step backwards in regards to maintainability as well as simplicity. I know these changes were triggered by the link you posted in the PR based on a suggestion by Graham Campbell. Bear in mind that the article in question as well as Campbell's comment are geared towards PHP frameworks, not packages. To me it doesn't make sense to implement all these interfaces as they have no real use case in this repo in particular. I can't think of any use case in your package that needs an exception to implement more than one interface.
The only reason I refactored my PR to comply with Campbell's suggestion is because a) I didn't think it did any harm as they are functionally the same, and b) to shut him up and keep the ball rolling. I've seen him pop into a dozen PRs, drop some fact as if it were the absolute truth (which more often than not throws a spoke in the wheel) and disappear, which is exactly what he did in this example. For many more examples of this behaviour go to the Laravel repo and you'll see what I mean.
To set the record straight I don't think his suggestion of "It is more usual to have exceptions implement [interfaces]" is even correct as in my experience it is the other way around. Having said that, either extending a base exception like I did originally on my PR or implementing one interface per exception as in the refactor, is more than enough for this package.
Personally, I'm not particularly fond of calling interfaces "contracts". An interface is an interface, that's it. Both the name and what they mean in PHP are simple to understand so I see no need to call them something else. It would be akin to calling variables "boxes" or functions "instruction sets". But again, this is just a personal preference! :)
@antonioribeiro commented on GitHub (Sep 12, 2019):
Released a v6 with it. Let's see what people say about this change, if anyone ever notice it.