fixed start-app false when generating openapi file

[spinettaro]

fixes #489 .
The fix implements a proper usage of OptionParser. When it uses boolean it is not able to understand assigned values like true or false but only if the key is present. The alternative is to use switches where you can prefix no to the existing key to switch the value to false or another approach is to handle it as a string and check for the passed value if it is =false. I implemented the one using swthces

[zorbash]

1. This would be a backwards incompatible change since there are already applications using the existing command-line options. Therefore we should be very cautious about merging it.
2. There's insufficient proof it addresses the root cause.

Specifically on the main branch:

Calling the following with a couple of IO.inspects:

(see: test/mix/tasks/openapi.spec.json_test.exs)

Mix.Tasks.Openapi.Spec.Json.run(~w(
  --quiet=true
  --start-app=false
  --spec OpenApiSpexTest.Tasks.SpecModule
  tmp/openapi.json
))

# The modified task with added inspect calls
def run(argv) do
    IO.inspect(argv, label: :ARGV)
    {opts, _, _} = IO.inspect(OptionParser.parse(argv, strict: [start_app: :boolean]), label: :OPTS)
   # omitted
end

Prints:

ARGV: ["--quiet=true", "--start-app=false", "--spec",
 "OpenApiSpexTest.Tasks.SpecModule", "tmp/openapi.json"]

OPTS: {[start_app: false], ["OpenApiSpexTest.Tasks.SpecModule", "tmp/openapi.json"],
 [{"--quiet", nil}, {"--spec", nil}]}

So start_app seems to be parsed correctly when set to false.

@spinettaro In the PR description you have:

When it uses boolean it is not able to understand assigned values like true or false but only if the key is present.

Can you please clarify what you mean? Why would it parse assigned values when the key is not present?

[spinettaro]

Hi @zorbash :)

1. I did not think about backward compatibility, good point. Let me think if there is a better way to do it, exploring also point 2)
2. You are right as it should work as expected. I thought that boolean was only able to detect the presence of the key and not the value assigned, that because even if it works for me when I run it directly
   

It does not work if I run the openapi.spec command

I need to understand why the above is not working, I'll keep you posted

[spinettaro]

ok, it seems it depends how you pass params. If we split start-app with the value like the following:

It does not work as you can see.

It reflects how ARGV are passed to openapi.spec command

[spinettaro]

@zorbash thoughts ?

In addition what made me think about switches rather than strict is OptionParser docs:

If you ask the tool to convert to ARGV bool: false it does not return --bool=false but --no-bool

[zorbash]

The problem you're facing with --start-app=false not working correctly seems to be powershell related.

With strict negation flags like --no-start-app should still work.

This is what phoenix uses here and here.

Example:

# File test.exs
OptionParser.parse(System.argv(), strict: [start_app: :boolean, other: :boolean])
|> IO.inspect()

% elixir test.exs --no-start-app                                                                                                                                                                              
# {[start_app: false], [], []}

Given there's a clear path to the desired behaviour without making any code change, I'm closing this.

[spinettaro]

@zorbash the problem is not related to powershell. It is not good to close this one, as the bug is still there. If you use the command provided start-app=false, it fails by starting the app and the tests do not simulate a real scenario; That is why they are passing

[spinettaro]

based on the reply on the official Elixir project elixir-lang/elixir#13197 (comment), this PR is more than valid and it is the proper way to do it.
We can skip backward compatibility as it never worked before: there is no backward comaptibility.

[zorbash]

based on the reply on the official Elixir project elixir-lang/elixir#13197 (comment), this PR is more than valid and it is the proper way to do it.

We can skip backward compatibility as it never worked before: there is no backward comaptibility.

Claiming that it never worked before is a strong statement unfortunately not backed by equally strong evidence. This can offend the people who have contributed this feature and anybody who has tested it.

You can test it on this replit which runs in on Linux and [email protected].

Your PR introduces unnecessary changes, specifically changing how options are parsed from strict (which is the preferred option - doc) to switches. This would have no impact to the problem you're trying to solve.

What's worth changing though is the docs, to suggest the recommended way of providing boolean flags, so mentioning --no-start-app as well as adding a test for it.

[spinettaro]

I'm sorry if my reply was misunderstood. I did not want to underestimate the work other people did before and I'm grateful of it. It was just in the context of backward compatibility. Given the fact it did not work, there is no reason to think about backward compatibility.

I demonstrated that start-app=false is not working and provided evidence of that. That is, unfortunately, the minimum evidence to say "it does not work". If it does not work even in one case, unfortunately, it does not work because there is at least one evidence of it.

That is not to underestimate the work done here, that is great! But to make it better, I think this is also the purpose of an open source project. We can discuss, collaborate, and decide how to make it better.

[spinettaro]

However, even on replit it does not work

The issue is that when they are passed as real cmd arguments, start-app and false are split in 2 different params, making not possible to assign a false value ti start-app as explained by @wojtekmach in the Elixir official project.

It worked for you because you were somehow hacking the arguments by passing something like ["start-app=false"] as unified params, but it cannot happen in real scenarios

[zorbash]

@spinettaro You're now accusing me of somehow hacking the arguments 🤯.

You can fork and inspect the code of the repl.it link I shared, there's nothing hacky going on.

There are also tests in the elixir codebase for this scenario, so please don't misquote Wojtek's comment who never said it's not possible to assign a false value.

[wojtekmach]

My bad, my answer wasn't precise was wrong. There's a subtle difference between --foo false and --foo=false, note the =, for booleans:

iex(1)> OptionParser.parse!(["--foo", "false"], strict: [foo: :boolean])
{[foo: true], ["false"]}
iex(2)> OptionParser.parse!(["--foo=false"], strict: [foo: :boolean])
{[foo: false], []}

[zorbash]

Thanks so much for chiming in @wojtekmach.

Just to clarify further, is the following by any means an Elixir / OptionParser anti-pattern or discouraged for booleans?

elixir main.exs --foo=false

[spinettaro]

Hi @zorbash ! Don't take it wrong, please :) I'm not trying to accuse anyone and something might go lost in translation as I'm not an English native speaker. No one is accusing you and my bad if you feel it. As you can see from Wojtekmach there was a misunderstanding on the answer and that caused a little bit of confusion.

When I wrote hacking your tests I meant open-api-spex tests and what I meant is that in your tests you assumed the form of arguments coming from cmd line. Getting for granted that syntax that produces arguments from cmd line o power shell or whatever is correct. And if you read the next point and you are right it is all good, but it is an assumption as you are not running a real cmd line but simulating how arguments are coming from it .

What I'm trying to solve is the following:

1. if elixir main.exs --foo=false is valid syntax, it should be resolved in terms of arguments as ["--foo=false"]. Then it is all good, happy days, something wrong in the Windows environment or whatever as they are passed in the wrong way, and open-api-spex is good. And the assumption you made about the syntax in the tests is correct!
2. if elixir main.exs --foo=false is not a valid syntax, then when it is resolved in terms of arguments as ["--foo", "false"], then it is an open-api-spex issue and we should try to fix it

Hopefully, it is more clear now and @wojtekmach can help with some inputs.