Replace case with with #245
Conversation
The case statement is too complex for situations when we want to focus on the success branch. This is the case of OptionParser.parse, which fails on invalid input, but requires complex parsing of correct switches and arguments.
lib/cli.ex
Outdated
| {:ok, module} = Map.fetch(@components, String.to_atom(component)) | ||
| working_dir = Keyword.get(parsed_switches, :working_dir, File.cwd!()) | ||
| module.main(working_dir) | ||
| parse_result = |
There was a problem hiding this comment.
parse should be enough, but at least parsed_result with d is more semantically right in my opinion.
There was a problem hiding this comment.
🤔 parsed_result doesn’t seem right to me: it’s not a result that has been parsed; it’s a result of parsing. parse_result tried to convey the message that it’s a result of the parse function.
Shortened to a single word, parsed may be a better choice. parse sounds like an action rather than a value.
lib/cli.ex
Outdated
|
|
||
| _ -> | ||
| usage_message() | ||
| with {parsed_switches, [component], []} <- parse_result do |
There was a problem hiding this comment.
There you can see that you call parsed_switches as parsed_switches but the result is not parsed only parse, it is not consistent.
There was a problem hiding this comment.
parse_result is a result of a parse function. parsed_switches on the other hand are really switches that have been parsed. The “result” has not been parsed – the args were and the “result” is the result of that.
I don’t really like the parsed_switches name. But since it’s there, I think the most consistent option for parse_result would be just parsed or parsed_argv.
For consistency with the parsed_switches variable.
|
|
The
casestatement is too complex for situations when we want to focus on the success branch. This is the case ofOptionParser.parse, which fails on invalid input, but requires complex parsing of correct switches and arguments.