-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add support for Rocket forms #445
Conversation
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.
Sorry for the delay in review! I like this - in fact, it's something I've been meaning to look into since I've been playing with Rocket for some personal projects.
My main comment is perhaps simplifying the feature name. It's really a nit, however I think it'll broaden the scope for any future functionality.
Default::default() | ||
} | ||
fn from_value(field: ValueField<'v>) -> form::Result<'v, Self> { | ||
Decimal::from_str(field.value).map_err(|_| form::Error::validation("not a valid number").into()) |
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.
Newbie question: do you need to do anything to handle empty strings? Or is that handled by Rocket if you mark the Decimal
as Option
?
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.
The latter.
If you require a Decimal
and from_str
(thus from_value
) fails, the request also fails (similary to how other types can fail, such as ints). If you require an Option<Decimal>
, values that fail from_value
become a None
.
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.
Oops my mistake. I've implemented default
, so when from_value
(if running on lenient mode, which is the default one) fails it is called and returns Decimal::default()
, which was not intended and could be misleading.
I'm gonna go ahead and remove that
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.
The only thing missing is running the tests within CI. This can be done by modifying Makefile.toml
- perhaps by creating a new test task and then adding it to test-all
. (Update: You can probably copy test-legacy-ops
as an example)
This just ensure that the tests are run on commit and future changes won't accidentally break this feature.
This should help with testing the CI checks too: https://github.com/paupino/rust-decimal/blob/master/BUILD.md#formatting--code-style |
Hello!
Here's a simple implementation for Rocket's
FromFormField
.This allows reading
Decimal
s straight from web form fields (either bodies or query strings).I'm not sure if the feature name is good enough, but i couldn't think of any other. I've also added a test. Should i add a failing one as well?