add enum wrapper #375
add enum wrapper #375
Conversation
src/JsonSchema/Enum.php
Outdated
|
|
||
| namespace JsonSchema; | ||
|
|
||
| class Enum extends \MabeEnum\Enum |
There was a problem hiding this comment.
Probably. Will give it a try when I get home.
There was a problem hiding this comment.
If this isn't intended to be extended-only (and might be used directly for something), then it doesn't need to be abstract, and probably shouldn't be. I was suggesting that based on an invalid assumption that this would always be extended for errors.
There was a problem hiding this comment.
Well that's the thing--I don't think that Enum or even Mabe/Enum has any utility without being extended. In fact, the base class itself is indeed abstract:
/~https://github.com/marc-mabe/php-enum/blob/master/src/Enum.php#L16
So I think you're right on the money on this one
There was a problem hiding this comment.
Gotcha - if that's the case, then yeah, it should be abstract.
src/JsonSchema/Enum.php
Outdated
| namespace JsonSchema; | ||
|
|
||
| class Enum extends \MabeEnum\Enum | ||
| { |
There was a problem hiding this comment.
This should declare an abstract method getMessage:
abstract public function getMessage();There was a problem hiding this comment.
Hmm, are you sure? Why would we assume that any other uses of Enum also have something to do with messages?
There was a problem hiding this comment.
Good point - I assumed based on your summary, but you do mention extending ConstraintError explicitly. Let's leave this PR as you originally had it :-).
There was a problem hiding this comment.
You're right. My code makes sense, my comment doesn't. A user would extend ConstraintError, not Mabe/Enum, so my fear of the unfamiliar namespace being exposed was not correct.
There was a problem hiding this comment.
If that's the case, what is the motivation for this PR? There's nothing that uses it other than ConstraintError, and if you aren't intending users to extend it either...
There was a problem hiding this comment.
More for internal benefit, I guess. If someone else decides to use Enum for something they can just extend /JsonSchema/Enum instead of /Mabe/Enum, and if we ever decide to switch to some other Enum implementation we only have to change the code in one place. Just a little future-proofing.
|
Rebased. Thanks for reviewing. |
Adding an intermediate
Enumclass. This way consumers of the library can extendConstraintErrorwithout having references to unfamiliar namespaces exposed in their code, and if we ever decide to go with a different enum implementation we don't have to worry (as much) about breaking their code.