CLI usage message #206
CLI usage message #206
Conversation
|
test/onigumo_cli_test.exs
Outdated
| use ExUnit.Case | ||
| import Mox | ||
| import ExUnit.CaptureIO |
There was a problem hiding this comment.
Can we group ExUnit.Case and ExUnit.CaptureIO? Not only it belongs together, but ExUnit comes alphabetically before Mox.
test/onigumo_cli_test.exs
Outdated
| output = capture_io(fn -> Onigumo.CLI.main([]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") | ||
| end | ||
|
|
||
| test("run CLI with more than one argument") do | ||
| assert_raise(MatchError, fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end) | ||
| output = capture_io(fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") | ||
| end | ||
|
|
||
| test("run CLI with invalid switch") do | ||
| assert_raise(OptionParser.ParseError, fn -> Onigumo.CLI.main(["--help"]) end) | ||
| output = capture_io(fn -> Onigumo.CLI.main(["--help"]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") |
There was a problem hiding this comment.
The capture_io and start_with? logic is quite complex and is repeated three times here. Could it be refactored to a private function acctepting just the fn?
lib/cli.ex
Outdated
| {[], [component]} = OptionParser.parse!(argv, strict: []) | ||
| component | ||
| rescue | ||
| _ -> usage_message() |
There was a problem hiding this comment.
I think that here we shoudl catch OptionParser.ParseError and MatchError. explicitly.
| _ -> usage_message() | |
| OptionParser.ParseError -> usage_message() | |
| MatchError -> usage_message() |
There was a problem hiding this comment.
I am thinking: to keep the amount of changes as small as possible, we can first only rescue the OptionParser.ParseError and then, in a separate pull request, extend the rescue for the MatchError.
Providing an invalid switch is actually a different user story from providing an invalid number of positional arguments. 🤷🏻♂️
test/onigumo_cli_test.exs
Outdated
| output = capture_io(fn -> Onigumo.CLI.main(["--help"]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") |
There was a problem hiding this comment.
If we only rescued OptionParser.ParseError, this would be the only amended test, reducing the amount of changes.
test/onigumo_cli_test.exs
Outdated
| output = capture_io(fn -> Onigumo.CLI.main([]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") | ||
| end | ||
|
|
||
| test("run CLI with more than one argument") do | ||
| assert_raise(MatchError, fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end) | ||
| output = capture_io(fn -> Onigumo.CLI.main(["Downloader", "Parser"]) end) | ||
| assert String.starts_with?(output, "Usage: ./onigumo ") |
There was a problem hiding this comment.
These two tests for an invalid number of arguments can be amended in a follow-up pull request, adding the MatchError.
lib/cli.ex
Outdated
| defp usage_message() do | ||
| IO.puts("Usage: ./onigumo [COMPONENT]\n") | ||
| IO.puts("\tSimple program that retrieve http web content in structured data.\n") | ||
| IO.puts("COMPONENT\tonigumo component to run, availables #{inspect(Map.keys(@components))}") |
There was a problem hiding this comment.
Does Elxir support some kind of heredocs?
IO.puts(<<<END)
Usage: ./onigumo [COMPONENT]
END
lib/cli.ex
Outdated
| defp usage_message() do | ||
| IO.puts("Usage: ./onigumo [COMPONENT]\n") | ||
| IO.puts("\tSimple program that retrieve http web content in structured data.\n") | ||
| IO.puts("COMPONENT\tonigumo component to run, availables #{inspect(Map.keys(@components))}") |
There was a problem hiding this comment.
Do we want to keep inspect? It exposes Elixir data structure that is only to be used for debugging. Maybe rather use Enum.join(", ", Map.keys(@components))?
Moreover, the expression is already pretty complex. It may deserve its own variable.
lib/cli.ex
Outdated
| defp usage_message() do | ||
| valid_components = Enum.join(Map.keys(@components), ", ") | ||
| IO.puts(""" | ||
| Usage: ./onigumo [COMPONENT] |
There was a problem hiding this comment.
Maybe the ./onigumo is too assuming in how the script is called.
| Usage: ./onigumo [COMPONENT] | |
| Usage: onigumo [COMPONENT] |
nappex
force-pushed
the
cli-help
branch
2 times, most recently
from
29deed0 to
632a586
Compare
fix #190