Adding mutually-exclusive fields functional validation #25
Conversation
Adding mechanism to validate for mutually-exclusive fields and initial fields that are mutually exclusive Fixes #21
|
@FokkeZB have I forgotten about any field combination? |
|
@BrunoBernardino shouldn't |
|
Oh, that's a good point, but it'll just be ignored, not cause errors, right? I'm trying to prevent those that can cause errors, otherwise we should also remove |
There was a problem hiding this comment.
I was really trying to think of better ways to structure this, and the closest I got was looking at: https://github.com/tdegrunt/jsonschema/blob/master/examples/all.js#L268 and https://github.com/tdegrunt/jsonschema/blob/master/examples/all.js#L315
Thinking it through, though, I think this is simpler (functional). Good test coverage, and straight forward code.
I've left some improvements, but nothing to block.
| const errors = []; | ||
|
|
||
| _.each(inputFields, (inputField, index) => { | ||
| _.each(incompatibleFields, (fieldGroup) => { |
There was a problem hiding this comment.
What version of node are we running again? 6.x would allow us to use destructure:
_.each(incompatibleFields, ([first, second]) => {
...
});| if (definition[typeOf]) { | ||
| _.each(definition[typeOf], (actionDef) => { | ||
| if (actionDef.operation && actionDef.operation.inputFields) { | ||
| errors = errors.concat(collectErrors(actionDef.operation.inputFields, `${typeOf}.${actionDef.key}`)); |
There was a problem hiding this comment.
errors = [...errors, ...collectErrors(...)]|
|
||
| _.each(inputFields, (inputField, index) => { | ||
| _.each(incompatibleFields, (fieldGroup) => { | ||
| if (inputField.hasOwnProperty(fieldGroup[0]) && inputField.hasOwnProperty(fieldGroup[1])) { |
There was a problem hiding this comment.
not sure if we need to worry about hasOwn but if so, let's use _.has which is just easier to read.
| // https://stackoverflow.com/questions/28162509/mutually-exclusive-property-groups#28172831 | ||
| // it was harder to read and understand. | ||
|
|
||
| const incompatibleFields = [ |
There was a problem hiding this comment.
I think this is simpler indeed than the stackoverflow :).
Would it be helpful if we added comments on why the combinations are not available (comments would suffice)?
There was a problem hiding this comment.
That's a good point, but screen estate is limited for the errors, there. Do you have any suggestions?
There was a problem hiding this comment.
Oh I meant inside the code.
|
@BrunoBernardino I agree priority number 1 is preventing errors, but ideally our schema should reject any specification that is invalid because a developer clearly added the property expecting to get something, but whatever it is he doesn't get it and that may lead to frustration or T2 tickets. |
Applying suggestions from code review. Related to #25
|
Can you please check again and see if I missed something @FokkeZB ? |
|
I might default to major because to the dev these combinations where
"expected". If you'd want minor we should output deprecation warnings.
|
|
LGTM @BrunoBernardino ! |
|
We may want to consider working with a separate upcoming-major branch (v3 now) to merge these breaking changes in. Then once we run into a urgent breaking change we ship that version, merge v3 into master and create a new major v4 branch. The only downside is that you need to merge master into the major branch every now and then. |
|
@BrunoBernardino If you used one of these invalid combos, what happened in the editor? Did it throw errors? Or did it just look odd but still work? If apps could function, we may have to do this as a major release. |
|
It didn't work well, but didn't throw errors. It seems we're agreeing on major, so that's what it'll be, then. There are no other breaking changes planned atm, so we don't need to hold this. |
BrunoBernardino
deleted the
feature-warn-on-mutually-exclusive-fields
branch
Adding mechanism to validate for mutually-exclusive fields and initial fields that are mutually exclusive
Fixes #21
Relevant Trello Card.
What this changes
childrenandlistchildrenanddictchildrenandtypechildrenandplaceholderchildrenandhelpTextchildrenanddefaultdictandlistdynamicanddictdynamicandchoicesLog
Adding mutually-exclusive fields functional validation (f00a778)
Adding mechanism to validate for mutually-exclusive fields and initial fields that are mutually exclusive
Fixes #21
Adding more mutually-exclusive fields to the checks (fbac4a0)
Applying suggestions from code review.
Related to #25