-
-
Notifications
You must be signed in to change notification settings - Fork 32
Form support for Email Field #286
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
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:
|
cot/src/form/types.rs
Outdated
@@ -0,0 +1,351 @@ | |||
//! Form Field Types for Cot |
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.
still deliberating on whether it's useful to have this file than put everything in fields.rs
(open to ideas on this)
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 it will be useful, but perhaps we could move it directly to crate::types
, given it's useful not only in forms, but also in auth. I'm not 100% sold on the name types
, though; it's very generic. I'm thinking about something in the lines of value_types
or data
.
@seqre do you have any additional views on this?
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 it will be useful, but perhaps we could move it directly to
crate::types
, given it's useful not only in forms, but also in auth.
I agree, Password
and Email
(and other similar types) are so common that it makes sense to extract them to their own place, so they can be reused in different situations.
I'm not 100% sold on the name
types
, though; it's very generic. I'm thinking about something in the lines ofvalue_types
ordata
.
I'm bad at naming as well, but some other ideas that come to my mind: common_types
, user_types
, or user_data
. I'd be against plain data
as it's too generic like types
.
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.
common_types
sounds good to me, but I also like value_types
🤔
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.
ended up going with common_types
. Anyone with an objection, speak now or forever hold your peace 😅
976f503
to
d7ee403
Compare
@m4tx I'm definitely doing something wrong, which makes the CI fail, that is not obvious to me. Do you have any pointers on what that may be? |
Yes, the |
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.
Overall looks good, please have a look at the comments and this will be ready to merge soon!
cot/src/form/types.rs
Outdated
@@ -0,0 +1,351 @@ | |||
//! Form Field Types for Cot |
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 it will be useful, but perhaps we could move it directly to crate::types
, given it's useful not only in forms, but also in auth. I'm not 100% sold on the name types
, though; it's very generic. I'm thinking about something in the lines of value_types
or data
.
@seqre do you have any additional views on this?
cot/src/form/types.rs
Outdated
/// assert_eq!(domain, "example.com"); | ||
/// ``` | ||
#[must_use] | ||
pub fn as_inner(&self) -> &EmailAddress { |
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.
Do you think it makes sense to remove this as_inner
method and instead expose the methods that could be potentially useful for users (like domain()
, local_part()
, etc.) instead? We generally try not to expose crates we depend on unnecessarily, as it makes it harder to swap these crates for different ones in the future, or even upgrade their versions (because every major version bump of an exposed crate results in a breaking change for us, too).
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.
EmailAddress
does not have many methods, so I'd agree on getting rid of as_inner
and just exposing those methods via our interface. An alternative option is to replace it with the implementation of Into<EmailAddress>
for it, which might be arguably a bit nicer to get rid of/deprecate in the future.
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.
Thanks for the suggestion—I think that makes sense. I originally added as_inner()
as an escape hatch for anyone needing extra functionality, but exposing the specific methods we actually use on Email
sounds great. We’ll just need to update those forwards if we ever swap out the underlying crate, which shouldn’t be too burdensome.
@seqre As for the Into<EmailAddress>
escape hatch, on second thought, my main concern is version conflicts: if consumers import EmailAddress
directly from the email_address
crate, bumping our dependency could suddenly break their builds. Do you think this will be a genuine concern, or do you propose we go with the first approach?
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 I ended up going with the first approach, I'm still unsure what the idiomatic approach is, or if the second approach is a genuine concern. If it isn't, let me know and I'll be happy to go in that direction.
cot/src/form/types.rs
Outdated
/// assert_eq!(domain, "example.com"); | ||
/// ``` | ||
#[must_use] | ||
pub fn as_inner(&self) -> &EmailAddress { |
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.
EmailAddress
does not have many methods, so I'd agree on getting rid of as_inner
and just exposing those methods via our interface. An alternative option is to replace it with the implementation of Into<EmailAddress>
for it, which might be arguably a bit nicer to get rid of/deprecate in the future.
cot/src/form/types.rs
Outdated
@@ -0,0 +1,351 @@ | |||
//! Form Field Types for Cot |
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 it will be useful, but perhaps we could move it directly to
crate::types
, given it's useful not only in forms, but also in auth.
I agree, Password
and Email
(and other similar types) are so common that it makes sense to extract them to their own place, so they can be reused in different situations.
I'm not 100% sold on the name
types
, though; it's very generic. I'm thinking about something in the lines ofvalue_types
ordata
.
I'm bad at naming as well, but some other ideas that come to my mind: common_types
, user_types
, or user_data
. I'd be against plain data
as it's too generic like types
.
6625487
to
5bb236b
Compare
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.
Looks good to me, thanks for this high-quality contribution!
I'll leave the last word to @seqre in case he has any outstanding comments, but from my side this should be ready to be merged.
if min > max { | ||
return Err(FormFieldValidationError::from_string(format!( | ||
"min_length ({min}) exceeds max_length ({max})" | ||
))); | ||
} |
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.
Ah, it would probably make sense to have some sort of mechanism to enforce this when constructing the field, rather than when cleaning the value. This is obviously out of scope of this PR, but I'll create an issue to keep this in the roadmap. EDIT: done #294
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.
Great work, just one last thing and I'll gladly approve!
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.
Once again, great work and thank you for your contribution!
aea98ff
to
e24d530
Compare
Add support for form email fields. This wraps over the
EmailAddress
type in the email_address crate.Also refactors and deprecates
cot::auth::Password
which is replaced bycot::form::types::Passsword
.Example usage: