| Message ID | 20190124232311.29636-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > It's not state in the documentation but optional arguments needs to be > specified using as '--foo=bar' instead of '--foo bar', otherwise the > value is not propagated to optarg during argument parsing. Update the > usage printing helper to reflect this requirement. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/options.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > --- a/src/cam/options.cpp > +++ b/src/cam/options.cpp > @@ -143,7 +143,8 @@ void OptionsParser::usage() > }; > > if (option.argument != ArgumentNone) { > - argument += std::string(" "); > + argument += option.argument == ArgumentOptional ? > + "=" : " "; > if (option.argument == ArgumentOptional) > argument += "["; > argument += option.argumentName; This will output -f, --foo value for mandatory arguments, and -f, --foo=[value] for optional arguments. If we want to print the =, shouldn't it be --foo[=value] ? And how should we handle the case where no long option is available, with this patch -f=[value] would be printed, which isn't correct I think.
Hi Laurent, On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: > Hi Niklas, > > On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > > It's not state in the documentation but optional arguments needs to be > > specified using as '--foo=bar' instead of '--foo bar', otherwise the > > value is not propagated to optarg during argument parsing. Update the > > usage printing helper to reflect this requirement. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/cam/options.cpp | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > > index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > > --- a/src/cam/options.cpp > > +++ b/src/cam/options.cpp > > @@ -143,7 +143,8 @@ void OptionsParser::usage() > > }; > > > > if (option.argument != ArgumentNone) { > > - argument += std::string(" "); > > + argument += option.argument == ArgumentOptional ? > > + "=" : " "; > > if (option.argument == ArgumentOptional) > > argument += "["; > > argument += option.argumentName; > > This will output > > -f, --foo value > > for mandatory arguments, and > > -f, --foo=[value] > > for optional arguments. If we want to print the =, shouldn't it be > --foo[=value] ? It should of course be --foo[=value], will fix. > And how should we handle the case where no long option > is available, with this patch -f=[value] would be printed, which isn't > correct I think. Good point, for optional short arguments the syntax would be -fvalue. Would it make sens to print both short and long syntax in the usage? -f[value], --foo[=value] Let me know what you think and I send a v2.
Heya, On 25/01/2019 11:23, Niklas Söderlund wrote: > Hi Laurent, > > On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: >> Hi Niklas, >> >> On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: >>> It's not state in the documentation but optional arguments needs to be >>> specified using as '--foo=bar' instead of '--foo bar', otherwise the >>> value is not propagated to optarg during argument parsing. Update the >>> usage printing helper to reflect this requirement. >>> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >>> --- >>> src/cam/options.cpp | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/src/cam/options.cpp b/src/cam/options.cpp >>> index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 >>> --- a/src/cam/options.cpp >>> +++ b/src/cam/options.cpp >>> @@ -143,7 +143,8 @@ void OptionsParser::usage() >>> }; >>> >>> if (option.argument != ArgumentNone) { >>> - argument += std::string(" "); >>> + argument += option.argument == ArgumentOptional ? >>> + "=" : " "; >>> if (option.argument == ArgumentOptional) >>> argument += "["; >>> argument += option.argumentName; >> >> This will output >> >> -f, --foo value >> >> for mandatory arguments, and >> >> -f, --foo=[value] >> >> for optional arguments. If we want to print the =, shouldn't it be >> --foo[=value] ? > > It should of course be --foo[=value], will fix. > >> And how should we handle the case where no long option >> is available, with this patch -f=[value] would be printed, which isn't >> correct I think. > > Good point, for optional short arguments the syntax would be -fvalue. > Would it make sens to print both short and long syntax in the usage? > > -f[value], --foo[=value] > > Let me know what you think and I send a v2. Is it difficult to accept a space or an = separator on the arguments?
Hi Kieran, On 2019-01-25 11:39:04 +0000, Kieran Bingham wrote: > Heya, > > On 25/01/2019 11:23, Niklas Söderlund wrote: > > Hi Laurent, > > > > On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: > >> Hi Niklas, > >> > >> On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > >>> It's not state in the documentation but optional arguments needs to be > >>> specified using as '--foo=bar' instead of '--foo bar', otherwise the > >>> value is not propagated to optarg during argument parsing. Update the > >>> usage printing helper to reflect this requirement. > >>> > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>> --- > >>> src/cam/options.cpp | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/src/cam/options.cpp b/src/cam/options.cpp > >>> index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > >>> --- a/src/cam/options.cpp > >>> +++ b/src/cam/options.cpp > >>> @@ -143,7 +143,8 @@ void OptionsParser::usage() > >>> }; > >>> > >>> if (option.argument != ArgumentNone) { > >>> - argument += std::string(" "); > >>> + argument += option.argument == ArgumentOptional ? > >>> + "=" : " "; > >>> if (option.argument == ArgumentOptional) > >>> argument += "["; > >>> argument += option.argumentName; > >> > >> This will output > >> > >> -f, --foo value > >> > >> for mandatory arguments, and > >> > >> -f, --foo=[value] > >> > >> for optional arguments. If we want to print the =, shouldn't it be > >> --foo[=value] ? > > > > It should of course be --foo[=value], will fix. > > > >> And how should we handle the case where no long option > >> is available, with this patch -f=[value] would be printed, which isn't > >> correct I think. > > > > Good point, for optional short arguments the syntax would be -fvalue. > > Would it make sens to print both short and long syntax in the usage? > > > > -f[value], --foo[=value] > > > > Let me know what you think and I send a v2. > > Is it difficult to accept a space or an = separator on the arguments? I think so, this is the default behavior of getopt_long() so without reimplementing that I think we are stuck with it. Maybe we can look into this once we are a bit further along and use the default behavior for now? > > > > -- > Regards > -- > Kieran
Hi Kieran, On Fri, Jan 25, 2019 at 12:23:25PM +0100, Niklas Söderlund wrote: > On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: > > On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > > > It's not state in the documentation but optional arguments needs to be > > > specified using as '--foo=bar' instead of '--foo bar', otherwise the > > > value is not propagated to optarg during argument parsing. Update the > > > usage printing helper to reflect this requirement. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/cam/options.cpp | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > > > index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > > > --- a/src/cam/options.cpp > > > +++ b/src/cam/options.cpp > > > @@ -143,7 +143,8 @@ void OptionsParser::usage() > > > }; > > > > > > if (option.argument != ArgumentNone) { > > > - argument += std::string(" "); > > > + argument += option.argument == ArgumentOptional ? > > > + "=" : " "; > > > if (option.argument == ArgumentOptional) > > > argument += "["; > > > argument += option.argumentName; > > > > This will output > > > > -f, --foo value > > > > for mandatory arguments, and > > > > -f, --foo=[value] > > > > for optional arguments. If we want to print the =, shouldn't it be > > --foo[=value] ? > > It should of course be --foo[=value], will fix. > > > And how should we handle the case where no long option > > is available, with this patch -f=[value] would be printed, which isn't > > correct I think. > > Good point, for optional short arguments the syntax would be -fvalue. > Would it make sens to print both short and long syntax in the usage? > > -f[value], --foo[=value] > > Let me know what you think and I send a v2. That would make sense, but then we should also write -f value, --foo value and we will lose the nice alignment of all long arguments :-( I'm not sure what's best.
Hello, On 2019-01-25 16:44:41 +0200, Laurent Pinchart wrote: > Hi Kieran, > > On Fri, Jan 25, 2019 at 12:23:25PM +0100, Niklas Söderlund wrote: > > On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: > > > On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > > > > It's not state in the documentation but optional arguments needs to be > > > > specified using as '--foo=bar' instead of '--foo bar', otherwise the > > > > value is not propagated to optarg during argument parsing. Update the > > > > usage printing helper to reflect this requirement. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > src/cam/options.cpp | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > > > > index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > > > > --- a/src/cam/options.cpp > > > > +++ b/src/cam/options.cpp > > > > @@ -143,7 +143,8 @@ void OptionsParser::usage() > > > > }; > > > > > > > > if (option.argument != ArgumentNone) { > > > > - argument += std::string(" "); > > > > + argument += option.argument == ArgumentOptional ? > > > > + "=" : " "; > > > > if (option.argument == ArgumentOptional) > > > > argument += "["; > > > > argument += option.argumentName; > > > > > > This will output > > > > > > -f, --foo value > > > > > > for mandatory arguments, and > > > > > > -f, --foo=[value] > > > > > > for optional arguments. If we want to print the =, shouldn't it be > > > --foo[=value] ? > > > > It should of course be --foo[=value], will fix. > > > > > And how should we handle the case where no long option > > > is available, with this patch -f=[value] would be printed, which isn't > > > correct I think. > > > > Good point, for optional short arguments the syntax would be -fvalue. > > Would it make sens to print both short and long syntax in the usage? > > > > -f[value], --foo[=value] > > > > Let me know what you think and I send a v2. > > That would make sense, but then we should also write > > -f value, --foo value > > and we will lose the nice alignment of all long arguments :-( I'm not > sure what's best. To move forward with this I would suggest, -f, --foo[=value] Rational being that if you use -f you get the default behavior and if you want to specify the optional argument you can use the long option. I would however not go out of the way to make -fvalue fail argument parsing. Then when we create a man page for the tool we can specify both versions in detail. Would this work for everyone? > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Fri, Jan 25, 2019 at 04:49:36PM +0100, Niklas Söderlund wrote: > On 2019-01-25 16:44:41 +0200, Laurent Pinchart wrote: > > On Fri, Jan 25, 2019 at 12:23:25PM +0100, Niklas Söderlund wrote: > >> On 2019-01-25 13:01:34 +0200, Laurent Pinchart wrote: > >>> On Fri, Jan 25, 2019 at 12:23:11AM +0100, Niklas Söderlund wrote: > >>>> It's not state in the documentation but optional arguments needs to be > >>>> specified using as '--foo=bar' instead of '--foo bar', otherwise the > >>>> value is not propagated to optarg during argument parsing. Update the > >>>> usage printing helper to reflect this requirement. > >>>> > >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>> --- > >>>> src/cam/options.cpp | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/src/cam/options.cpp b/src/cam/options.cpp > >>>> index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 > >>>> --- a/src/cam/options.cpp > >>>> +++ b/src/cam/options.cpp > >>>> @@ -143,7 +143,8 @@ void OptionsParser::usage() > >>>> }; > >>>> > >>>> if (option.argument != ArgumentNone) { > >>>> - argument += std::string(" "); > >>>> + argument += option.argument == ArgumentOptional ? > >>>> + "=" : " "; > >>>> if (option.argument == ArgumentOptional) > >>>> argument += "["; > >>>> argument += option.argumentName; > >>> > >>> This will output > >>> > >>> -f, --foo value > >>> > >>> for mandatory arguments, and > >>> > >>> -f, --foo=[value] > >>> > >>> for optional arguments. If we want to print the =, shouldn't it be > >>> --foo[=value] ? > >> > >> It should of course be --foo[=value], will fix. > >> > >>> And how should we handle the case where no long option > >>> is available, with this patch -f=[value] would be printed, which isn't > >>> correct I think. > >> > >> Good point, for optional short arguments the syntax would be -fvalue. > >> Would it make sens to print both short and long syntax in the usage? > >> > >> -f[value], --foo[=value] > >> > >> Let me know what you think and I send a v2. > > > > That would make sense, but then we should also write > > > > -f value, --foo value > > > > and we will lose the nice alignment of all long arguments :-( I'm not > > sure what's best. > > To move forward with this I would suggest, > > -f, --foo[=value] > > Rational being that if you use -f you get the default behavior and if > you want to specify the optional argument you can use the long option. I > would however not go out of the way to make -fvalue fail argument > parsing. Then when we create a man page for the tool we can specify both > versions in detail. > > Would this work for everyone? Works for me.
diff --git a/src/cam/options.cpp b/src/cam/options.cpp index 82acff9bbeea476d..73d81d0bc0ec6d38 100644 --- a/src/cam/options.cpp +++ b/src/cam/options.cpp @@ -143,7 +143,8 @@ void OptionsParser::usage() }; if (option.argument != ArgumentNone) { - argument += std::string(" "); + argument += option.argument == ArgumentOptional ? + "=" : " "; if (option.argument == ArgumentOptional) argument += "["; argument += option.argumentName;
It's not state in the documentation but optional arguments needs to be specified using as '--foo=bar' instead of '--foo bar', otherwise the value is not propagated to optarg during argument parsing. Update the usage printing helper to reflect this requirement. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/options.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)