Skip to content

Fix bug with default type + cargo fmt#304

Merged
Keats merged 12 commits into
masterfrom
fix-bugs
Apr 5, 2024
Merged

Fix bug with default type + cargo fmt#304
Keats merged 12 commits into
masterfrom
fix-bugs

Conversation

@Keats

@Keats Keats commented Mar 9, 2024

Copy link
Copy Markdown
Owner

No description provided.

@kyrias

kyrias commented Mar 25, 2024

Copy link
Copy Markdown
Contributor

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

Keats commented Mar 25, 2024

Copy link
Copy Markdown
Owner Author

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.

/// Don't put a & in front a pointer since we are going to pass
/// a reference to the validator
/// Also just use the ident without if it's optional and will go through
/// a if let first
pub fn quote_validator_param(&self) -> proc_macro2::TokenStream {
let ident = &self.ident;
if self._type.starts_with("Option<") {
quote!(#ident)
} else if COW_TYPE.is_match(self._type.as_ref()) {
quote!(self.#ident.as_ref())
} else if self._type.starts_with('&') || NUMBER_TYPES.contains(&self._type.as_ref()) {
quote!(self.#ident)
} else {
quote!(&self.#ident)
}
}

I should probably port that back though

@Keats

Keats commented Mar 26, 2024

Copy link
Copy Markdown
Owner Author

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

@kyrias

kyrias commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

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

kyrias commented Mar 27, 2024

Copy link
Copy Markdown
Contributor

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

Keats commented Mar 27, 2024

Copy link
Copy Markdown
Owner Author

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

kyrias commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

Fixed in #315.

* Dynamically build the list of number types

Round-tripping these through `quote!().to_token_stream().to_string()`
lets us ensure that the resulting strings always match.

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>

* Don't destructure number types by reference

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>

* Add test for custom fns on number types not taking a reference

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>

---------

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>
@Keats

Keats commented Mar 29, 2024

Copy link
Copy Markdown
Owner Author

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

@kyrias

kyrias commented Apr 2, 2024

Copy link
Copy Markdown
Contributor

Everything else looks good to me.

@Keats Keats merged commit ba8a9b9 into master Apr 5, 2024
@Keats Keats deleted the fix-bugs branch April 5, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants