Usg msg reason #275

nappex · 2025-04-11T20:24:59Z

…e evaluation

Glutexo

Thanks for the pull request! ❤️

Comments marked as ❗️ are blocking. The other ones are more of comments and suggestions.

The two suggested Enum.join(invalid, ", ") occurrences may be DRIED into a function.

  defp join_for_usage_message(args):
    Enum.join(args, ", ")

lib/cli.ex

Co-authored-by: Glutexo <[email protected]>

lib/cli.ex

Glutexo · 2025-04-22T16:39:34Z

lib/cli.ex

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

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


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

Created: #278

Co-authored-by: Glutexo <[email protected]>

lib/cli.ex

test/onigumo_cli_test.exs

Glutexo · 2025-04-22T17:01:28Z

test/onigumo_cli_test.exs

-        assert usage_message_printed?(fn -> Onigumo.CLI.main(unquote(combination)) end)
+        assert usage_message_printed?(
+                 fn -> Onigumo.CLI.main(unquote(combination)) end,
+                 # there is no easy way to do it dynamically


Yes, this sounds fishy.

The constant can be amended to contain the expected output.

@invalid_combinations [ {["--help", "-C", ".", "downloader"], ["--help"], {["downloader", "-h"], ["--help"]}, {["downloader", "--help"], ["--help"]}, ]

--help is the only incompatible option though. That means the second element is unnecessary.

We can however omit the comment: there is an easy way to do it, it’s just not necessary.

Suggested change

# there is no easy way to do it dynamically

Suggested change

# there is no easy way to do it dynamically

# The incompatible argument is always --help.

Another thing came to my mind, however. It’s how we always call help to be the the incompatible one and never --working-dir. It makes sense to some extent: a component defines a set of valid switches containing --working-dir and not --help. Thus, it’s --help that’s incompatible. But it may become more complicated if the CLI becomes more convoluted.

lib/cli.ex

Glutexo

I pick Request changes for only two reasons:

The # there is no easy way to do it dynamically comment doesn’t feel appropriate. There is and easy way. Either do it, change the comment to be more descriptive, or omit it at all.
The @invalid_switches extension deserves a preliminary PR that would unblock this one.

Glutexo · 2025-04-25T13:29:31Z

lib/cli.ex

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


Clever! I like the conciseness of the &-function returning directly a string.

The expression is maybe too complex. What about extracting the Enum.join/2 call?

Suggested change

|> then(&"incompatible OPTIONS #{Enum.join(&1, ", ")}")

|> Enum.join(", ")

|> then(&"incompatible OPTIONS #{&1}")

lib/cli.ex

Glutexo · 2025-04-25T13:38:45Z

lib/cli.ex

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


Let’s dissect this too.

Suggested change

|> then(&"invalid OPTIONS #{Enum.join(&1, ", ")}")

|> Enum.join(", ")

|> then(&"invalid OPTIONS #{&1}")

Alrigth why not, it looks like more clear and legibility, thank you.

Glutexo · 2025-04-25T14:59:16Z

lib/cli.ex

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

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


Created: #278

Glutexo · 2025-04-25T14:59:44Z

lib/cli.ex

    IO.write("""
-    onigumo: invalid usage
+    onigumo: invalid usage – #{reason}


Glutexo · 2025-04-25T15:45:58Z

test/onigumo_cli_test.exs

-        assert usage_message_printed?(fn -> Onigumo.CLI.main(unquote(combination)) end)
+        assert usage_message_printed?(
+                 fn -> Onigumo.CLI.main(unquote(combination)) end,
+                 # there is no easy way to do it dynamically


The constant can be amended to contain the expected output.

@invalid_combinations [ {["--help", "-C", ".", "downloader"], ["--help"], {["downloader", "-h"], ["--help"]}, {["downloader", "--help"], ["--help"]}, ]

--help is the only incompatible option though. That means the second element is unnecessary.

We can however omit the comment: there is an easy way to do it, it’s just not necessary.

Suggested change

# there is no easy way to do it dynamically

Suggested change

# there is no easy way to do it dynamically

# The incompatible argument is always --help.

Another thing came to my mind, however. It’s how we always call help to be the the incompatible one and never --working-dir. It makes sense to some extent: a component defines a set of valid switches containing --working-dir and not --help. Thus, it’s --help that’s incompatible. But it may become more complicated if the CLI becomes more convoluted.

Glutexo · 2025-04-25T15:47:08Z

test/onigumo_cli_test.exs

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

    for combination <- @invalid_combinations do


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.

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.

Glutexo · 2025-04-25T17:29:00Z

test/onigumo_cli_test.exs

+        assert usage_message_printed?(
+                 fn -> Onigumo.CLI.main(unquote(combination)) end,
+                 # there is no easy way to do it dynamically
+                 "incompatible OPTIONS --help"


Suggested change

"incompatible OPTIONS --help"

"incompatible OPTIONS #{invalid}"

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

Glutexo · 2025-04-25T17:30:13Z

test/onigumo_cli_test.exs

      output = capture_io(function)
-      String.starts_with?(output, "onigumo: invalid usage")
+      String.starts_with?(output, "onigumo: invalid usage – #{reason}")


Just thinking out loud.

Suggested change

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

message = "onigumo: invalid usage – #{reason}"

String.starts_with?(output, message)

yes it is more clean suppose

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

Glutexo · 2025-04-25T17:33:23Z

test/onigumo_cli_test.exs

-    "-c"
+    ["--invalid"],
+    ["-c"],
+    ["-a", "-b"]


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

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

Add argument reason with default value nil to usage message

b888dee

nappex force-pushed the usg-msg-reason branch from 7bfa28b to 5738b1d Compare April 12, 2025 15:57

nappex added 3 commits April 12, 2025 18:14

Insert reasons

8ba245d

Do not use nil as default instead use empty string, trim reason befor…

3c177c7

…e evaluation

Add reason to tests

aaa31c7

nappex force-pushed the usg-msg-reason branch from 5738b1d to aaa31c7 Compare April 12, 2025 16:14

nappex marked this pull request as ready for review April 12, 2025 16:15

Glutexo requested changes Apr 16, 2025

View reviewed changes

lib/cli.ex Outdated Show resolved Hide resolved

Glutexo assigned nappex Apr 17, 2025

Reason is argument, update resons content

2068810

Glutexo reviewed Apr 18, 2025

View reviewed changes

lib/cli.ex Outdated Show resolved Hide resolved

Reason is more descriptive

9372499