mpeterv / argparse

Feature-rich command line parser for Lua
MIT License
251 stars 43 forks source link

Print default values even if non string #9

Open achalddave opened 8 years ago

achalddave commented 8 years ago

I'm not sure if I'm missing something in the documentation, but it seems that default values are not printed unless they are strings. I understand this may not be possible with complicated types, but with primitives such as doubles, this would be nice to have in the output of --help.

mpeterv commented 7 years ago

Hi @achalddave, sorry for such a late reply. Default values are always strings, if you pass a non-string it gets forwarded to init option instead, which has slightly different meaning - it sets initial value for an option that's not passed through actions and coverters. In hindsight overloading an option like that is rather strange, but the point is that you should pass string representation of your default number to argparse, and if you use :convert(tonumber) argparse will convert the argument to number whether it's the default one or it came from the user.

ghost commented 7 years ago

This also affects whether a value is treated as being optional or not.

I.e. take the following example:

local argparse = require "argparse"

function open_if_string(filename)
  if type(filename) == "string" then
    return io.open(filename)
  end
  return filename
end

local parser = argparse()
parser:argument "input"
        :description("Input file")
        :args(1)
        :default("default.file")
        :convert(io.open)
parser:argument "output"
        :description("Output file")
        :args(1)
        :default(io.stdout)
        :convert(open_if_string)

local args = parser:parse()

Calling this with -h results in the following output (stripped):

Usage: test.lua [-h] [<input>] <output>

output is not optional as expected, even though input, which looks virtually the same except having a different default value type, is indeed optional.

While using "STDOUT" as the default value and then converting that to the actual io.stdout in the converter is possible, it will break in unexpected ways when someone actually tries to output to a file called "STDOUT".

My proposal is to extend the :default(value) function to not only take the value, but optionally also a string representation and behave the same no matter what type value has. This would look like this:

parser:argument "input"
        :description("Input file")
        :args(1)
        :default("default.file") -- just like above
        :convert(io.open)
parser:argument "output"
        :description("Output file")
        :args(1)
        :default(io.stdout, "STDOUT") -- with an optional 2nd argument
        :convert(open_if_string)

The output for -h would be:

Arguments:
   input                 Input file (default: default.file)
   output                Output file (default: STDOUT)
ghost commented 7 years ago

This gets even better: Apparently default values are completely ignored, if the argument is marked as optional with :args("?"):

local argparse = require "argparse"

local parser = argparse()
parser:argument "input"
        :description("Input file")
        :args("?")
        :default("default.file")
        :convert(error)

local args = parser:parse()
print(args.input)
# lua test-convert.lua
nil

If I swap :args("?") for :args(1) to make the argument required, I get the expected stacktrace, thrown from convert by calling my error "converter".

mpeterv commented 7 years ago

While using "STDOUT" as the default value and then converting that to the actual io.stdout in the converter is possible, it will break in unexpected ways when someone actually tries to output to a file called "STDOUT".

Typically a dash (-) is used to represent stdout or stdin.

My proposal is to extend the :default(value) function to not only take the value, but optionally also a string representation and behave the same no matter what type value has.

It can't always behave the same, it has to apply converter to string defaults but not to other types.

What do you think about using show_default method to pass string representation of default value instead of adding second argument to default?

This gets even better: Apparently default values are completely ignored, if the argument is marked as optional with :args("?"):

This is working as expected, if an argument is optional, there is no need to pass a default value to it.

ghost commented 7 years ago

Typically a dash (-) is used to represent stdout or stdin.

Thanks for the reminder. I'll do that for now.

It can't always behave the same, it has to apply converter to string defaults but not to other types.

Why does it have to behave in this way? Actually I was very surprised when I saw that it did not apply the converter to non-string values. I was under the assumption that it would always be my converter's task to convert the value into the form I desire, whether it originally was a string or not. So my first try after reading the documentation was simply to put the logic into the converter: If it is a string, assume it's a filename and open it; if it is already a file, just pass it on.

This is working as expected, if an argument is optional, there is no need to pass a default value to it.

Is this documented somewhere?

mpeterv commented 7 years ago

It can't always behave the same, it has to apply converter to string defaults but not to other types.

Why does it have to behave in this way? Actually I was very surprised when I saw that it did not apply the converter to non-string values. I was under the assumption that it would always be my converter's task to convert the value into the form I desire, whether it originally was a string or not. So my first try after reading the documentation was simply to put the logic into the converter: If it is a string, assume it's a filename and open it; if it is already a file, just pass it on.

So the difference would be having if type(v) ~= "string" then return v end in all converters instead of doing that check once somewhere in the library? What are the advantages of that?

Also, this is consistent with Python's argparse:

If the default value is a string, the parser parses the value as if it were a command-line argument. In particular, the parser applies any type conversion argument, if provided, before setting the attribute on the Namespace return value. Otherwise, the parser uses the value as is.

This is working as expected, if an argument is optional, there is no need to pass a default value to it.

Is this documented somewhere?

Looks like it isn't, I'll fix that. Currently the tutorial says:

the default value will be automatically passed to the element if it was not invoked at all.

But it doesn't say how many time it will be passed. And it will be passed minimum possible number of times, so, 0 with :args "?".

ghost commented 7 years ago

So the difference would be having if type(v) ~= "string" then return v end in all converters instead of doing that check once somewhere in the library? What are the advantages of that?

My idea was that this makes the library more flexible for different use-cases and developer expectations. (Because you give them exactly one thing and in all cases.)

Also, this is consistent with Python's argparse:

If the default value is a string, the parser parses the value as if it were a command-line argument. In particular, the parser applies any type conversion argument, if provided, before setting the attribute on the Namespace return value. Otherwise, the parser uses the value as is.

Well, I'm also fine with that. Just make this behaviour very obvious in the documentation. And fix the :args(1):default(...) case.

Under these conditions I'd even go so far as to not support non-string values at all and just raise an error in this case. It would help with consistency and be much more obvious to the programmer with regards to what the expected input, output and behaviour are.

I'm also fine with staying consistent with Python regarding non-string :default()s. It's surprising that they did not come up with a solution for the default value being missing in --help, since they support that in the first case, but I understand your decision to follow their lead. (Which of course also means you'll have to support non-string :default() values, contradictory to what I suggested above.)