WIP attempt to implement more general range support #4034
Conversation
src/builder/value_parser.rs
Outdated
| /// let port: u16 = *m.get_one("port").expect("required"); | ||
| /// assert_eq!(port, 3001); | ||
| /// ``` | ||
| fn in_range<R: RangeBounds<Self::Value>>(self, range: R) -> InRange<Self> |
There was a problem hiding this comment.
Unsure how I feel about this being on the trait.
There was a problem hiding this comment.
Yeah, I don't love it either. Some other options would be a standalone function or macro (ex range_parser!(u16: 3000..4000)). Or maybe creating a different trait for types that can be restricted to a range?
src/builder/value_parser.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn in_range<R: RangeBounds<P::Value>>(self, range: R) -> InRange<P> |
There was a problem hiding this comment.
The current behavior is if you do value_parser!(u16).range(1..)
- It gets parsed as a i64 internally, causing more parse errors to be treated as range errors
- The ranges compound with each other so you get
1..=u16::MAX
With the current implementation,
- It doesn't allow narrowing the value's type, causing a regression in error messages
- It doesn't merge the ranges
There was a problem hiding this comment.
hmm. For the second part, that could potentially be resolved by require T to implement a trait that exposes the maximum and minimum values. Although that would require the type to actually have a minimum and maximum value (or maybe it returns an Option?), and of course implement the trait for any meaningful data types, such as the integral and floating point types in the standard library.
The first part is more difficult though. If we wanted to restrict this to integers, then possibly the current implementation could be changed to use i128 and u128 on platforms that support it. But i'm not sure there is any garantee that those will be at least as big as isize and usize. It also doesn't allow specifying a range for f32 or f64. Nor would it really be able to support custom types that have an ordering (although I'm not sure how much of a need there is for that).
There was a problem hiding this comment.
My original attempt at this had generic types for the parse type and the value type and required TryInto to be supported between them.
|
Any updates on this or #3895? |
Still very rough, and needs a lot of work
This is still very much a work in progress, but I'd like to put it up to see if this is a direction worth pursuing.
See #3895