feat: Added derive macro SelectAsFormField#397
Conversation
github-actions
bot
added
C-lib
Codecov Report❌ 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:
|
|
@m4tx can you take a look whenever you get the time. |
There was a problem hiding this comment.
Thanks for your patience waiting for my review! As usual, the change looks good, but I have some minor comments about stuff that needs to be fixed before merging this.
Also, please make sure to add additional tests if needed so that the patch has at least 85% code coverage.
About the impl_as_form_field_mult macro—since it's private and is now unused, I don't see a value in keeping it, and so it should be removed.
cot/src/form/fields/select.rs
Outdated
| impl<T: SelectChoice + Send> crate::form::AsFormField for ::std::vec::Vec<T> { | ||
| type Type = SelectMultipleField<T>; | ||
|
|
||
| fn clean_value(field: &Self::Type) -> Result<Self, FormFieldValidationError> { | ||
| let values = check_required_multiple(field)?; | ||
| values.iter().map(|id| T::from_str(id)).collect() | ||
| } | ||
|
|
||
| fn to_field_value(&self) -> String { | ||
| String::new() | ||
| } | ||
| } | ||
|
|
||
| impl<T: SelectChoice + Send> crate::form::AsFormField for ::std::collections::VecDeque<T> { | ||
| type Type = SelectMultipleField<T>; | ||
|
|
||
| fn clean_value(field: &Self::Type) -> Result<Self, FormFieldValidationError> { | ||
| let values = check_required_multiple(field)?; | ||
| let mut out = ::std::collections::VecDeque::new(); | ||
| for id in values { | ||
| out.push_back(T::from_str(id)?); | ||
| } | ||
| Ok(out) | ||
| } | ||
|
|
||
| fn to_field_value(&self) -> String { | ||
| String::new() | ||
| } | ||
| } | ||
|
|
||
| impl<T: SelectChoice + Send> crate::form::AsFormField for ::std::collections::LinkedList<T> { | ||
| type Type = SelectMultipleField<T>; | ||
|
|
||
| fn clean_value(field: &Self::Type) -> Result<Self, FormFieldValidationError> { | ||
| let values = check_required_multiple(field)?; | ||
| let mut out = ::std::collections::LinkedList::new(); | ||
| for id in values { | ||
| out.push_back(T::from_str(id)?); | ||
| } | ||
| Ok(out) | ||
| } | ||
|
|
||
| fn to_field_value(&self) -> String { | ||
| String::new() | ||
| } | ||
| } | ||
|
|
||
| impl<T: SelectChoice + Eq + ::std::hash::Hash + Send, S: ::std::hash::BuildHasher + Default> | ||
| crate::form::AsFormField for ::std::collections::HashSet<T, S> | ||
| { | ||
| type Type = SelectMultipleField<T>; | ||
|
|
||
| fn clean_value(field: &Self::Type) -> Result<Self, FormFieldValidationError> { | ||
| let values = check_required_multiple(field)?; | ||
| let mut out = ::std::collections::HashSet::default(); | ||
| for id in values { | ||
| out.insert(T::from_str(id)?); | ||
| } | ||
| Ok(out) | ||
| } | ||
|
|
||
| fn to_field_value(&self) -> String { | ||
| String::new() | ||
| } | ||
| } | ||
|
|
||
| impl<T: SelectChoice + Eq + ::std::hash::Hash + Send> crate::form::AsFormField | ||
| for ::indexmap::IndexSet<T> | ||
| { | ||
| type Type = SelectMultipleField<T>; | ||
|
|
||
| fn clean_value(field: &Self::Type) -> Result<Self, FormFieldValidationError> { | ||
| let values = check_required_multiple(field)?; | ||
| let mut out = ::indexmap::IndexSet::new(); | ||
| for id in values { | ||
| out.insert(T::from_str(id)?); | ||
| } | ||
| Ok(out) | ||
| } | ||
|
|
||
| fn to_field_value(&self) -> String { | ||
| String::new() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Why can't we use the impl_as_form_field_mult_collection macro for these implementations? They are all essentially the same, and as such, prone to mistakes when you want to modify something in one impl and forget to change all the others. We just need to another variant of the macro so that it supports generic bounds.
As an added bonus, Iterator::collect() might be slightly faster than <Collection>::push_back because of preallocation, although admittedly, this likely doesn't matter for the typical element counts in case of HTML form values.
There was a problem hiding this comment.
I have made the changes. Please take a look if this is what you have in mind.
Co-authored-by: Mateusz Maćkowski <[email protected]>
m4tx
left a comment
m4tx
left a comment
There was a problem hiding this comment.
Hey! This looks good - please fix the only remaining issue and we'll merge this.
m4tx
left a comment
m4tx
left a comment
There was a problem hiding this comment.
A good change, as usual, from you; thank you so much for this!
2 participants
As per the requirements of the #356 I implemented the
SelectAsFormFieldderive macro. There were two directions for this either creating a derive macro or create a wrapper struct. Since I love derive macro and comfortable implementing it, I went with that.After this change the
impl_as_form_field_multmacro is now unused, I would like to know what is the preference for that?