Miscellaneous cli-related parity fixes#5442
Conversation
decf6a8 to
cccbd2e
Compare
cccbd2e to
e4be882
Compare
82fd867 to
c6da4ff
Compare
| debug!("Running command {}", command); | ||
| vm.run_code_string(scope, &command, "<stdin>".to_owned())?; | ||
| vm.run_code_string(scope.clone(), &command, "<stdin>".to_owned()) | ||
| .map(drop) |
There was a problem hiding this comment.
Is this a common convention to drop value from Result? It looks like drop is important logic here to me. If not, a weak suggestion.
| .map(drop) | |
| .map(|_| ()) |
There was a problem hiding this comment.
I've seen .map(drop) a decent amount, yeah.
| env::var_os(name).filter(|v| !v.is_empty()).map(|value| { | ||
| value | ||
| .to_str() | ||
| .and_then(|v| v.parse::<u8>().ok()) | ||
| .unwrap_or(1) | ||
| }) |
There was a problem hiding this comment.
| env::var_os(name).filter(|v| !v.is_empty()).map(|value| { | |
| value | |
| .to_str() | |
| .and_then(|v| v.parse::<u8>().ok()) | |
| .unwrap_or(1) | |
| }) | |
| let value_str = env::var_os(name).filter(|v| !v.is_empty())?; | |
| let value = value_str | |
| .to_str() | |
| .and_then(|v| v.parse::<u8>().ok()) | |
| .unwrap_or(1); | |
| Some(value) |
There was a problem hiding this comment.
Given there's 2 layers of Option here, I think I'd like to keep the logic as it is - looking at the suggested version my instinct was to change it to .to_str()?.parse().unwrap_or(1), but that's different. I think the explicit map() makes more clear the control flow and conditions.
There was a problem hiding this comment.
line 349 and the following lines are unrelated logics. early return reveals line 350 is independent to 349 and code readers can forget about 349 when going to 350.
Using map make readers essentially follow the full logic without benefit.
Part of this is taken from #5414.