-
Notifications
You must be signed in to change notification settings - Fork 144
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
Expose validator constraints to end-users using a new trait #238
Conversation
Thanks for the PR. I'm actually a bit torn on how to set up constraints. I don't want third party crates like As for the PR itself, I don't think there will be much changes (other than a rewrite of the derive crate) so I think I would leave it as it for now and revisit when that rewrites happen. |
I don't care about third-party crates, but not implementing the crate for the third-party crates that this crate already supports would be a breaking change, as it would break every crate currently using them.
I think that would be a mistake as those are semantically disparate things. Right now if you try to do |
Yep that would be a breaking change.
Right now there are some very ugly checks (https://github.com/Keats/validator/blob/master/validator_derive/src/asserts.rs#L8-L61 and the rest of the files) which fail entirely as soon as it's a unsupported crate or even just a type alias. I'm still not entirely clear on how that would work, maybe having the separate traits do make sense in the end but we need to remove those asserts hacks. That's part of the reason for the rewrite, and also to handle new attributes like |
So I'm not really sure whether there's anything you want changed here currently? Do you want me to file another PR to remove support for the third-party crates? |
No I think the first step is to rewrite the validation logic in the vein of #225 + a rewrite of the derive macro to support things like |
I'd love this to get merged but I'm not exactly sure I understand what is blocking this right now. So if I read this correctly, it's blocked on #225 which is currently also not really getting worked on? Is there some clear path forward here and can it be assisted somehow? |
* implemented validation trait for length * converted identation to spaces * changed the trait to not require HasLen * added macro for generating impls * implemented ValidateLength for some types * using trait validation instead of the function * added cfg for indexmap import * changed trait to require length * Revert "changed trait to require length" This reverts commit a77bdc9. * moved validation logic inside ValidateLength trait * added trait validation for required * added email trait validation * fixed trait validation for email * added range trait validation * fixed range trait * added url trait validation --------- Co-authored-by: Tilen Pintarič <tilen.pintaric@aviko.si>
Keats#246) * feat(range): add exclusive minimum and exclusive maximum for `range` validation * test(range): add tests for `exc_min` and `exc_max` range validation * docs: add docs for `exc_min` and `exc_max` for `range` validation * chore: rename `exc_min`, `exc_max` to `exclusive_min`, `exclusive_max` * chore(validation.rs): get rid of `collide` function
…kind This makes it possible for code consuming the constraints to know whether the type is used directly or if it is held in some kind of collection.
This PR exposes all of the validator crate constraints defined for a specific type through a new trait called
Constraints
. The new trait only contains associated methods and so does not pose a backwards-compatibility issue.We have tests that ensure that expected valid and invalid values will not be erroneously accepted/rejected. To do this we currently double-encode these constraints on both the actual types and then again in the test suite.
This is not scalable, and it's very hard to ensure that they're always kept in sync. So instead we want to be able to just ask the validator crate which constraints are defined for a specific type.