- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 33
 
[WIP]Move FormField's custom option validation to field construction #304
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
base: master
Are you sure you want to change the base?
Conversation
          Codecov ReportAttention: Patch coverage is  
 
 Flags with carried forward coverage won't be shown. Click here to find out more. 
 ... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
  | 
    
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub trait ValidateOptions { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a new trait for this isn't ideal. The problem is that it has to be implemented for each Options struct, and it's not even very useful for the end users—note we only use it in a helper macro (not even a public one). This means that the documentation "The validation is performed when FormField::with_options is called" is not really true, as you can always provide your own with_options implementation.
I can see the following alternatives, in the order of my preference:
- Changing the macro so that it can optionally call 
validate()or some similar method (not defined in any trait) on the options struct, if needed (the macro call would change to something likeimpl_form_field!(StringField, StringFieldOptions, "a string", with_validate)for the structs that need to be validated, fro instance. - Making the trait private (there is no promise for the users, so there's no problem, but we still have to add 
implfor every options struct) - Making the helper macro public (but there still is no obligation to use the macro, so the trait promise is still broken)
 
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like option 2 the most and it might not require as much work in the end, we could incorporate the default implementation into the impl_form_field! macro, but leave an escape hatch for custom validation.
Go from this:
macro_rules! impl_form_field {
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?) => {
        ...original impl...
}To this:
macro_rules! impl_form_field {
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?) => {
        impl_form_field!(...arguments from above..., custom-validation)
        impl ValidateOptions for $field_options_type_name {}
}
    ($field_type_name:ident, $field_options_type_name:ident, $purpose:literal $(, $generic_param:ident $(: $generic_param_bound:ident $(+ $generic_param_bound_more:ident)*)?)?, custom-validation) => {
        ...original impl...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seqre how do you specify custom validation code with your approach? ah, I see the custom-validation part. In this case, it's okay (although I don't have a strong preference of 1 or 2).
fixes #294