-
Notifications
You must be signed in to change notification settings - Fork 129
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
Create new error abstraction for field validation #239
Conversation
f93a1be
to
7a28525
Compare
16eb5b9
to
7b8e719
Compare
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.
Looks good, other than my comment around error constants, let me know if you want to chat about the best pattern before changing anything.
6e2d33e
to
d0140e5
Compare
@phamann changes made, waiting re-review |
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.
👍🏻 LGTM
Will you be incorporating the other PR into this release?
@phamann yes. the author replied to me eariler to say they would take a look next week. So with that in mind. I can merge this change but not cut a release until after they've managed to wrap up any other changes and write some tests in their PR. |
d0140e5
to
33c924a
Compare
cfabd53
to
72e149f
Compare
e1c3545
to
dc644df
Compare
@fastly/developer-relations @phamann I've reverted a bunch of approved changes (see the latest commits above). I'm going to open a few separate PRs to home these changes (as they've already been approved by Patrick they should be quick to review/approve) but it'll help ensure the dynamically generated CHANGELOG will have explicit callouts to each change (whereas if I kept them all within this PR they would be lost). |
Create new error abstraction for field validation. This helps with consistency and maintainability.