-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat(union): add details to union errors #276
base: master
Are you sure you want to change the base?
feat(union): add details to union errors #276
Conversation
Thanks I really appreciate this, but changing the failcode is a breaking change. Let me postpone merging this until the next major version. P.S. I'm not saying that we shouldn't change the failcode. |
ebc6ba7
to
199d3cb
Compare
BREAKING CHANGE: Union failcode has changed from TYPE_INCORRECT to CONTENT_INCORRECT. Fixes runtypes#275
788588b
to
d927809
Compare
@johngeorgewright Thanks rebasing this, but basically I don't want to require PR authors to do it by themselves. I'm working on other tasks, and I believe you will feel it tiresome if you need rebasing/merging upstream every time any update occurs. Please leave it to me 😉 |
else if (validation.details) details.push(validation.details); | ||
else details.push(validation.message); | ||
} | ||
return FAILURE.CONTENT_INCORRECT(self, details); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. What if we split these changes into 2 PRs?
PR1: return code: TYPE_INCORRECT
but with details
PR2: change code
to CONTENT_INCORRECT
In this case PR1 does not have breaking changed, it can be merged in minor release
return FAILURE.CONTENT_INCORRECT(self, details); | |
const fail = FAILURE.CONTENT_INCORRECT(self, details); | |
// TODO: remove this code hack in next major release | |
fail.code = Failcode.TYPE_INCORRECT; | |
return fail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuhr what do you think?
would you merge this PR if it preserves code: TYPE_INCORRECT
but still adding details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, sounds neat but how we should format the error message might differ between TYPE_INCORRECT
and CONTENT_INCORRECT
(not sure but probably), so let me proceed #296 before taking action on this. Thanks suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something that can be revisited? As more versions are published, it gets harder to keep up-to-date using a fork in a git reference.
This is a simple workaround for #275 providing details on all validation errors.