Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace case with with #245

Closed
wants to merge 3 commits into from
Closed

Replace case with with #245

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86554bc
Replace case with with
Glutexo Nov 9, 2024
e3a0cee
Rename parse_result to parsed
Glutexo Nov 9, 2024
33d65f8
Merge branch 'master' into with
Glutexo Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions lib/cli.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,15 @@ defmodule Onigumo.CLI do
}

def main(argv) do
case OptionParser.parse(
argv,
aliases: [C: :working_dir],
strict: [working_dir: :string]
) do
{parsed_switches, [component], []} ->
{: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 =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse should be enough, but at least parsed_result with d is more semantically right in my opinion.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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.

OptionParser.parse(argv, aliases: [C: :working_dir], strict: [working_dir: :string])

_ ->
usage_message()
with {parsed_switches, [component], []} <- parse_result do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There you can see that you call parsed_switches as parsed_switches but the result is not parsed only parse, it is not consistent.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{:ok, module} = Map.fetch(@components, String.to_atom(component))
working_dir = Keyword.get(parsed_switches, :working_dir, File.cwd!())
module.main(working_dir)
else
_ -> usage_message()
end
end

Expand Down
Loading