Implement dict and list-of-scalar parsing for options sources. #20428
Conversation
| @@ -77,6 +79,31 @@ impl Args { | |||
| } | |||
| Ok(None) | |||
| } | |||
|
|
|||
| fn get_list<T>( | |||
There was a problem hiding this comment.
A generic variant of the old get_string_list, which is now reimplemented using this (as are the other scalar list types). The parse_list argument is the specific parser function we should apply to the raw string to yield the needed list type.
| @@ -77,7 +77,7 @@ fn test_bool() { | |||
| #[test] | |||
| fn test_float() { | |||
| let args = args([ | |||
| "-j=4", | |||
| "-j=4.0", | |||
There was a problem hiding this comment.
We never documented support for integer literals as floats, and we expect (and now only support) Python float literals.
There was a problem hiding this comment.
Hmm, maybe we should support this because people may be relying on it in practice?
There was a problem hiding this comment.
Yeah, I'd be very unsurprised if people are doing things like setting options like pantsd_timeout_when_multiple_invocations to 120. It'd be annoying to be required to specify the .0. Is it hard to support integer literals too?
There was a problem hiding this comment.
It's tricksy to do it in the parser, because we don't want to "swallow" actual ints as floats in a dict. But we can post-process safely I think.
There was a problem hiding this comment.
Yep, we can coerce an int in get_float safely. Done.
| assert_float(3.14, option_id!("foo")); | ||
| assert_float(1.137, option_id!("baz", "spam")); | ||
|
|
||
| assert!(args.get_float(&option_id!("dne")).unwrap().is_none()); | ||
|
|
||
| assert_eq!( | ||
| "Problem parsing --bad value swallow as a float value: invalid float literal".to_owned(), | ||
| "Problem parsing --bad float value:\n1:swallow\n ^\n\ |
There was a problem hiding this comment.
We switch to using the parse.rs methods for everything, including scalars, for uniformity (and to support non-rust literals that python supports, such as underscores in ints and floats)
| @@ -305,6 +335,57 @@ impl Config { | |||
| .and_then(|table| table.get(Self::option_name(id))) | |||
| } | |||
|
|
|||
| fn get_list<T: FromValue>( | |||
There was a problem hiding this comment.
Again, a genericised get_string_list
| self.get_list(id, parse_string_list) | ||
| } | ||
|
|
||
| fn get_dict(&self, id: &OptionId) -> Result<Option<DictEdit>, String> { |
There was a problem hiding this comment.
The diff here is confusing - this is a brand new function
| @@ -66,6 +69,20 @@ impl Env { | |||
| } | |||
| names | |||
| } | |||
|
|
|||
| fn get_list<T>( | |||
There was a problem hiding this comment.
Again, a generic version of get_string_list
| @@ -77,7 +77,7 @@ fn test_bool() { | |||
| #[test] | |||
| fn test_float() { | |||
| let args = args([ | |||
| "-j=4", | |||
| "-j=4.0", | |||
There was a problem hiding this comment.
Yeah, I'd be very unsurprised if people are doing things like setting options like pantsd_timeout_when_multiple_invocations to 120. It'd be annoying to be required to specify the .0. Is it hard to support integer literals too?
| remove, | ||
| )?, | ||
| }) | ||
| if sub_table.len() == 1 && add.is_table() { |
There was a problem hiding this comment.
What happens if these aren't true? It looks like the value is silently dropped?
There was a problem hiding this comment.
What happens if these aren't true? It looks like the value is silently dropped?
Ah yes, good catch. We should probably never have supported a subtable named "add" the way we do for lists, because unlike in the list case this is also a valid dict value. But we do in Python so we must here as well.
Anyway, fixed.
No description provided.