Skip to content

Usg msg reason #275

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.

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
25 changes: 17 additions & 8 deletions lib/cli.ex
Original file line number Diff line number Diff line change
Expand Up @@ -21,25 +21,34 @@ defmodule Onigumo.CLI do
{working_dir, switches} = Keyword.pop(switches, :working_dir, File.cwd!())

case switches do
[] -> module.main(working_dir)
_ -> usage_message()
[] ->
module.main(working_dir)

_ ->
OptionParser.to_argv(switches)
|> Enum.join(", ")
|> then(&"incompatible OPTIONS #{&1}")
|> usage_message()
end

:error ->
usage_message()
usage_message("invalid COMPONENT #{component}")
end

{_, _, [_ | _]} ->
usage_message()
{_, _, invalid = [_ | _]} ->
Enum.map(invalid, &elem(&1, 0))
|> Enum.join(", ")
|> then(&"invalid OPTIONS #{&1}")
|> usage_message()

{_, argv, _} when length(argv) != 1 ->
Copy link
Owner

Choose a reason for hiding this comment

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

This guard is probably unnecessary, I‘ll create an issue for that.

Copy link
Owner

Choose a reason for hiding this comment

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

Created: #278

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK

usage_message()
usage_message("exactly one COMPONENT must be provided")
end
end

defp usage_message() do
defp usage_message(reason) do
IO.write("""
onigumo: invalid usage
onigumo: invalid usage – #{reason}
Copy link
Owner

Choose a reason for hiding this comment

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

– ❤️

Usage: onigumo [OPTION]... [COMPONENT]

Try `onigumo --help' for more options.
Expand Down
39 changes: 29 additions & 10 deletions test/onigumo_cli_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ defmodule OnigumoCLITest do
]

@invalid_switches [
"--invalid",
"-c"
["--invalid"],
["-c"],
["-a", "-b"]
Copy link
Owner

Choose a reason for hiding this comment

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

🤔 I am not sure about this. It adds the possibility to test joining for the usage message. But this test should have been there in the first place. Or at least list of invalid switches instead of a single value. I feel it doesn’t belong into this PR.

However, without the list support and this test in place, it is not possible to test the new feature.

Copy link
Collaborator Author

@nappex nappex Apr 27, 2025

Choose a reason for hiding this comment

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

I would keep it maybe in different format, but you have to test a new feature in this PR

]

@invalid_combinations [
Expand All @@ -32,27 +33,42 @@ defmodule OnigumoCLITest do
describe("Onigumo.CLI.main/1") do
for argument <- @invalid_arguments do
test("run CLI with invalid argument #{inspect(argument)}") do
assert usage_message_printed?(fn -> Onigumo.CLI.main([unquote(argument)]) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main([unquote(argument)]) end,
"invalid COMPONENT #{unquote(argument)}"
)
end
end

test("run CLI with no arguments") do
assert usage_message_printed?(fn -> Onigumo.CLI.main([]) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main([]) end,
"exactly one COMPONENT must be provided"
)
end

test("run CLI with more than one argument") do
assert usage_message_printed?(fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end,
"exactly one COMPONENT must be provided"
)
end

for switch <- @invalid_switches do
test("run CLI with invalid switch #{inspect(switch)}") do
assert usage_message_printed?(fn -> Onigumo.CLI.main([unquote(switch)]) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main(unquote(switch)) end,
"invalid OPTIONS #{Enum.join(unquote(switch), ", ")}"
)
Comment on lines +59 to +62
Copy link
Owner

Choose a reason for hiding this comment

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

Here, we can however simplify this single expression a bit.

Suggested change
assert usage_message_printed?(
fn -> Onigumo.CLI.main(unquote(switch)) end,
"invalid OPTIONS #{Enum.join(unquote(switch), ", ")}"
)
switches = Enum.join(unquote(switch), ", ")
assert usage_message_printed?(
fn -> Onigumo.CLI.main(unquote(switch)) end,
"invalid OPTIONS #{switches}"
)

I can imagine going even further.

Suggested change
assert usage_message_printed?(
fn -> Onigumo.CLI.main(unquote(switch)) end,
"invalid OPTIONS #{Enum.join(unquote(switch), ", ")}"
)
func = fn -> Onigumo.CLI.main(unquote(switch)) end
message = "invalid OPTIONS #{Enum.join(unquote(switch), ", ")}"
assert usage_message_printed?(func, message)

That would however need to be consistent and done for all tests.

I don’t insist on doing this.

end
end

for combination <- @invalid_combinations do
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
for combination <- @invalid_combinations do
for {combination, invalid} <- @invalid_combinations do

If the constant contained tuples as imagined below, the parametrization would look like this.

Copy link
Collaborator Author

@nappex nappex Apr 27, 2025

Choose a reason for hiding this comment

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

I am not sure if it is just one value, it feels like unnecessary DRY at this moment.
Ok, it is not so difficult to dynamically solve this problem so, the comment is redundant and should be deleted.

test("run CLI with invalid combination #{inspect(combination)}") do
assert usage_message_printed?(fn -> Onigumo.CLI.main(unquote(combination)) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main(unquote(combination)) end,
"incompatible OPTIONS --help"
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"incompatible OPTIONS --help"
"incompatible OPTIONS #{invalid}"

If the constant contained tuples as imagined above, the string would look like this.

)
end
end

Expand All @@ -73,7 +89,10 @@ defmodule OnigumoCLITest do
end

test("run CLI 'downloader' with #{inspect(switch)} without any value") do
assert usage_message_printed?(fn -> Onigumo.CLI.main(["downloader", unquote(switch)]) end)
assert usage_message_printed?(
fn -> Onigumo.CLI.main(["downloader", unquote(switch)]) end,
"invalid OPTIONS #{unquote(switch)}"
)
end
end

Expand All @@ -96,9 +115,9 @@ defmodule OnigumoCLITest do
String.match?(output, ~r/\N\n\z/)
end

defp usage_message_printed?(function) do
defp usage_message_printed?(function, reason) do
output = capture_io(function)
String.starts_with?(output, "onigumo: invalid usage")
String.starts_with?(output, "onigumo: invalid usage – #{reason}")
Copy link
Owner

Choose a reason for hiding this comment

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

Just thinking out loud.

Suggested change
String.starts_with?(output, "onigumo: invalid usage – #{reason}")
message = "onigumo: invalid usage – #{reason}"
String.starts_with?(output, message)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it is more clean suppose

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but then it is for another PR since the same way of solving is for help message

end

defp help_message_printed?(function) do
Expand Down
Loading