Keats / validator

Simple validation for Rust structs
MIT License
1.91k stars 140 forks source link

Fix bug with default type + cargo fmt #304

Closed Keats closed 3 months ago

kyrias commented 4 months ago

By the way, this seems to only partially restore the custom function behavior. You no longer need to be able to handle the Option types, but the function signature has switched from T to &T. Was this intentional? For example, we have some Option<i16> fields where the function previously accepted an i16 but now needs to accept &i16. This isn't actually a problem for us, just wanted to make sure that it's an intentional divergence. :)

Keats commented 4 months ago

That's because the previous version was doing some "smart" arg passing: in practice if it was a std number type it was passing by copy vs reference. https://github.com/Keats/validator/blob/60614b5e2a940022385203e0d5abc943683d97c5/validator_derive/src/quoting.rs#L26-L42

I should probably port that back though

Keats commented 4 months ago

That should work again as before if you want to try it

kyrias commented 4 months ago

It doesn't work for optional number types for two reasons:

  1. The wrapper_closure does an if let Some(ref ...) which adds one layer of referencing anyway.

  2. The NUMBER_TYPES array hasn't been updated to include the extra spacing that gets included when you do .to_token_stream().to_string().

    It might actually make sense to build the array of number types using .to_token_stream().to_string() to ensure that they can't get out of sync.

kyrias commented 4 months ago

Simple compile-pass test:

fn foo(_: i16) -> Result<(), ValidationError> {
    Ok(())
}

#[derive(Validate)]
struct NumberTypesCustomValidation {
    #[validate(custom(function = "foo"))]
    plain: i16,
    #[validate(custom(function = "foo"))]
    option: Option<i16>,
    #[validate(custom(function = "foo"))]
    option_option: Option<Option<i16>>,
}
Keats commented 4 months ago

Ah yes of course. I'll fix it when i have time or if someone wants to fix it and i'll merge it. After this i think we can release 0.18 and we should be back to where we were in 0.16 from usage pov (minus the nested being required and require_nested being removed and just using required, nested as validators

kyrias commented 4 months ago

Fixed in #315.

Keats commented 3 months ago

Ok I believe with that we can release 0.18? Anything else causing issues?

kyrias commented 3 months ago

Everything else looks good to me.