Fix Merge issues between custom and nested #359
Conversation
|
One comment here is it could make sense to join it in a list rather than the map we currently have with nested/self. Would require more logic but It shouldn't be too difficult. Would like thoughts on this though so I put up the simplest clean solution |
|
Without my limited knowledge of this codebase, this seems like a reasonable short-term solution. Agree that joining it in a list instead of the current map would make sense. Thanks for taking a stab at it! |
validator/src/types.rs
Outdated
| let mut new_table = ValidationErrors::new(); | ||
| new_table.0.insert("self", entry.clone()); | ||
| new_table.0.insert("nested", errors); | ||
| self.0.insert(field, ValidationErrorsKind::Struct(Box::new(new_table))); |
There was a problem hiding this comment.
I think rather than changing that, it would be better to redesign the whole error types so you only get 1 breaking change instead of 2 with that new change
There was a problem hiding this comment.
Would need a bit more explanation on how a redesign would cause less breaking changes. I don't see how this would cause 2 breaking changes? (I would argue that we can consider this 0 breaking changes for SemVer since all this does is fixes a panic bug case which mean the current behavior when this exact case happens is undefined?)
There was a problem hiding this comment.
Right now it panics so it doesn't work and people don't rely on it. If we merge that, people might start to rely on the self/nested fields which would be one more breaking change compared to if we did not merge that.
I'll try to sketch what the new errors would look like this afternoon
There was a problem hiding this comment.
Oh you mean in the future if we change. Got it. I thought you meant 2 breaking changes with this PR. I should be able to help with the implementation if you have a new idea for errors
There was a problem hiding this comment.
@Keats what do you suggest as our path forward here?
There was a problem hiding this comment.
Looking at #357, we should basically not be in a position to have both custom and nested.
In
#[validate(nested, custom(function = "all_value1s_are_unique"))]
a: Vec<A>,
we should run the custom function first and only if it passes run the nested validations
There was a problem hiding this comment.
Sorry for delay, I think this should be done now. I have the extra case where if it is missed and we did run the nested and custom has errors, we ignore the nested too.
pvichivanives
force-pushed
the
master
branch
2 times, most recently
from
e61885a to
62beb09
Compare
pvichivanives
force-pushed
the
master
branch
from
62beb09 to
83c21a7
Compare
validator/src/types.rs
Outdated
| } | ||
| // Else case here is simply do nothing. Pretty sure this is caught earlier to not run but since | ||
| // If we get here, just not adding it does the same thing (although waste extra compute) | ||
| // probably better to just ignore here. |
There was a problem hiding this comment.
I think the panic is helpful to have in case we are missing cases. At least it's going to point out another bug
There was a problem hiding this comment.
Sure! I'll add the panic back. Does mean a higher chance of us having another bug.
pvichivanives
force-pushed
the
master
branch
from
ee1c5aa to
d0a500d
Compare
pvichivanives
force-pushed
the
master
branch
from
d0a500d to
3bb3e92
Compare
A response that should fix #357
Did not include the test in this PR since it should be merged in from #357
Slight behavior change in that it we dont run nested if custom has an error. its a bug fix should be called a patch/minor? (Given that this behavior is better than a panic)
Called an unwrap here to show error output from #357
