Pass globals as command-line options #78
Conversation
|
cc @BekaValentine — this might be convenient for you. |
|
I'm ok with leaving both the "how do we handle non- |
| version = "2.32" | ||
| version = "3.2" |
There was a problem hiding this comment.
Had to bump this to be able to supply the args the way I wanted.
| .short("q") | ||
| .short('q') |
There was a problem hiding this comment.
Consequence of bumping clap.
| let globals = matches.get_many::<String>("global").unwrap_or_default(); | ||
| let mut globals_ = Variables::new(); | ||
| for kv in globals { | ||
| let kv_ = kv | ||
| .split_once('=') | ||
| .with_context(|| format!("Expected key-value pair separated by '=', got {}.", kv))?; | ||
| globals_.add( | ||
| Identifier::from(kv_.0), | ||
| graph::Value::String(kv_.1.to_string()), | ||
| )?; | ||
| } |
There was a problem hiding this comment.
I would have preferred to do this with a single map but I couldn't figure out how to express the two nested levels of failure (failure in parsing global values, and failure in the surrounding context). If anyone has suggestions, I'm all ears; I think we should be able to avoid both the second variable and the mutability, and (ideally) end up with something more readable as well.
There was a problem hiding this comment.
I actually like how you've done it here! As you point out, it becomes messy with the two levels of error handling, and I think this rendering makes the control flow clearer.
(It's doable, and probably involves the collect method, which takes a vec of results and turns it into a result of a vec. But again, I'd rather we keep it as you've written it here.)
There was a problem hiding this comment.
(Also, if you want to bound the region where globals_ is mutable, you can do something like this:
let globals_ = globals_;after you're done mutating it. That makes it immutable for the remainder of the scope.)
There was a problem hiding this comment.
I actually like how you've done it here! As you point out, it becomes messy with the two levels of error handling, and I think this rendering makes the control flow clearer.
(It's doable, and probably involves the
collectmethod, which takes a vec of results and turns it into a result of a vec. But again, I'd rather we keep it as you've written it here.)
Cool, if this is idiomatic then I'll be happy to 
(Also, if you want to bound the region where
globals_is mutable, you can do something like this:let globals_ = globals_;after you're done mutating it. That makes it immutable for the remainder of the scope.)
Oh, that's a lovely use of shadowing! Thank you.
| let mut config = ExecutionConfig::new(&mut functions, &globals).lazy(lazy); | ||
| let mut config = ExecutionConfig::new(&mut functions, &globals_).lazy(lazy); |
There was a problem hiding this comment.
This is the key to how this all works: instead of passing in an empty map of Variables, we pass in the one we built above instead. Really quite tidy, that.
| let globals = matches.get_many::<String>("global").unwrap_or_default(); | ||
| let mut globals_ = Variables::new(); | ||
| for kv in globals { | ||
| let kv_ = kv | ||
| .split_once('=') | ||
| .with_context(|| format!("Expected key-value pair separated by '=', got {}.", kv))?; | ||
| globals_.add( | ||
| Identifier::from(kv_.0), | ||
| graph::Value::String(kv_.1.to_string()), | ||
| )?; | ||
| } |
There was a problem hiding this comment.
I actually like how you've done it here! As you point out, it becomes messy with the two levels of error handling, and I think this rendering makes the control flow clearer.
(It's doable, and probably involves the collect method, which takes a vec of results and turns it into a result of a vec. But again, I'd rather we keep it as you've written it here.)
|
Nice improvement @robrix! |
How should types other thanSee Pass globals of non-string types on CLI #86.Stringbe handled?String::split_once()seems not to match when the rhs (after the delimiter) is empty. How should we handle empty globals, e.g. empty strings?