Feedback

[leontoeides]

Just a few comments! Just some feedback for your consideration -

1. The custom fn signature requires the second context parameter. While I think this is a powerful feature, it's not really necessary for me in most cases so far. Perhaps there could be two custom validation function types? One with context and one without? I feel like creating numerous functions with an second parameter that I never use isn't very elegant.

2. When I have an Option wrapped type, it looks like garde is passing the full Option<T> to the custom validator function. Whereas validator will only pass T. It would seem that garde is forcing me to validate None. I suppose there could be a context where a None value is invalid? On the other hand, the validator way seems to be - it will attempt to validate an unwrapped Some but it won't bother validating None. I personally feel this is better (for me). The garde setup also requires me to have two validators: one for Option<T> and one for T.

garde is your crate, and I'm sure you designed it the way you saw best. Just offering some food for thought.

Thank you again for the great crate!

[jprochazk]

Thanks, I appreciate the feedback :).

The custom fn signature requires the second context parameter.

I agree that this is inconvenient at best. Especially the &() in every validate call. I think because the derive macro knows whether or not there is a context, it could call the function with just the value if there is no custom context:

#[derive(Validate)]
struct Foo {
    #[garde(custom(baz))] // no context provided, called with just `bar`
    bar: String,
}

fn baz(v: &str) -> garde::Result {
    todo!()
}

#[derive(Validate)]
#[garde(context(MyContext))]
struct Foo2 {
    #[garde(custom(baz))] // context provided, called with `bar, ctx`
    bar: String,
}

fn baz2(v: &str, ctx: &MyContext) -> garde::Result {
    todo!()
}

Another option is to change how custom validators work by introducing a Custom<T, Context> trait. It would be blanket implemented for all Fn(&T, &Context) and Fn(&T). This is similar to how frameworks like axum deal with handlers that have a variable number of extractors.

That being said, I'm not sure how I would get rid of the &() from the top-level validate call other than by introducing a second trait, which I really want to avoid. I also don't want to require Context: Default or similar. Would love to know if you have any ideas :).

When I have an Option wrapped type, it looks like garde is passing the full Option to the custom validator function

The custom validator should receive the full value every time, it simplifies it quite a bit. That doesn't mean I don't think your use case is valid! I have plans to introduce an inner modifier for rules (#23 (comment)) which should do what you want:

#[derive(Validate)]
struct Foo {
    #[garde(inner(custom(baz)))]
    bar: Option<String>,
}

fn baz(v: &str) -> garde::Result {
    todo!()
}

The inner modifier would use a trait implemented for various containers, including Option. For Option specifically, it would match on it, and only call baz if it is Some.

[leontoeides]

Great response sir. These both seem like good solutions to me! If the derive macro is able to determine the correct function signature to use, I'm sure that would be the better approach. (I've done no work with Rust macros myself, I'm not familiar with the tools at your disposal.)

I'm not sure that I agree that passing an unwrapped Option is best if you're able to detect that there's no Context. To me, there's no value in having to unwrap a value to validate it. Conversely, there's no value in validating a None. Still - inner works for me too, and I'll take gladly take it. 😏

[lasantosr]

That being said, I'm not sure how I would get rid of the &() from the top-level validate call other than by introducing a second trait, which I really want to avoid. I also don't want to require Context: Default or similar. Would love to know if you have any ideas :).

@jprochazk You could only requiere the Default trait for the context in the validate method without it, seems reasonable to me:

pub trait Validate {
    type Context;

    fn validate(&self) -> Result<(), Report>
    where
        Self::Context: Default,
    {
        let ctx = Self::Context::default();
        self.validate_with(&ctx)
    }

    fn validate_with(&self, ctx: &Self::Context) -> Result<(), Report> {
        ...
    }

    fn validate_into(
        &self,
        ctx: &Self::Context,
        parent: &mut dyn FnMut() -> Path,
        report: &mut Report,
    );
}

[jprochazk]

Last part of this will be resolved by:

• Remove need for ctx param for custom validators on context-less structs #104

I'll keep this one closed as it's easier to track the work with an issue dedicated to it.