schema/types: don't panic validating incomplete isolators #633
Conversation
|
It turns out rkt has some tests doing exactly what godoc says not to do. I'm going to fix those tests, but this makes validation a bit more graceful. I'm not sure if there is an easy way to forbid this at all without making the type hierarchy even more complex. |
|
LGTM. I have no problem with making it fail more gracefully in this case of user-error. |
|
Ideally ValueRaw would not be exported at all, but we're hamstrung there by Go's json marshalling rules |
| @@ -24,6 +24,7 @@ var ( | |||
| isolatorMap map[ACIdentifier]IsolatorValueConstructor | |||
|
|
|||
| ErrIncompatibleIsolator = errors.New("isolators set contains incompatible types") | |||
| ErrInvalidIsolator = errors.New("invalid isolator") | |||
There was a problem hiding this comment.
Could you add a docstring for this error? hinting that it might be thrown if an Isolator is constructed incorrectly for example
There was a problem hiding this comment.
Yup, the one above too? In general I haven't seen lot of docstring around error constants, but I will be happy to have more.
There was a problem hiding this comment.
Yeah - in this case in particular I think it helps because as a consumer of the package it's unclear why I'd ever encounter this (it's unfortunate that we allow them to even get into this situation)
lucab
force-pushed
the
to-upstream/isolator-value
branch
from
9845f2f to
0ae6774
Compare
|
Amended. |
| ErrIncompatibleIsolator = errors.New("isolators set contains incompatible types") | ||
| // ErrInvalidIsolator is thrown upon validation failures due to improper | ||
| // or partially constructed Isolator instances (ie. not constructed via unmarshaling) |
There was a problem hiding this comment.
The ie is wrong (since valid isolators can be constructed from the New* functions).
Also errors are returned, not thrown.
Even though Isolator instances should be constructed via unmarshaling, library users may still construct one directly with a nil value. This commit fixes the validator to handle this error gracefully instead of panicing.
lucab
force-pushed
the
to-upstream/isolator-value
branch
from
0ae6774 to
d3cde07
Compare
Even though Isolator instances should be constructed via
unmarshaling, library users may still construct one directly
with a nil value.
This commit fixes the validator to handle this error gracefully
instead of panicing.