Add support for Rocket forms #445
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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
Decimals 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?