Consider adding named accessor methods

[adishavit]

There has been a request to add named accessor methods in addition to operator[] and operator(). See Twitter thread by @azadkuh here. Though simple to do, there are several subtle aspects that need to be addressed. I will use this issue to contemplate, document and discuss them.

I have an (as yet non-public) experimental branch with this, but I'm not yet sure about this. (cc: @arnemertz)

[azadkuh]

following our discussion in twitter:

• constness at the moment the accessors operators are non-const, it would be great if you make them const as they return const & or a value.

• operator[] at the moment this operator both checks if a flag is set and also access an argument's value by index which is a little bit confusing. i suggest this operator should only return an argument value (by index or name)

  
  std::string operator[](size_t index) const; // as std::vector, std::string, std::array, QVector, QList, ...
  std::string operator[](std::string const& name) const; // as std::map, QHash, QMap, ...

// also like std or Qt containers std::string at(size_t index) const; std::string at(std::string const& name) const; // or template T value(std::string const& name, T&& default_value) const;

- add `is_set()`
to check if a flag is set, this method seems to be a better choice than overloading `operator[]`:
```cpp
bool is_set(std::string const& name) const;
bool is_set(std::initializer_list<char const* const> init_list) const;

Qt QCommandLineParser has also a similar method.

• operator() at the moment this operator returns an string_stream. IMHO this operator mostly resembles a call operator / functor / ... may be a named function may help:

  template<class... Args>
  string_stream to_stream(Args&&... args) const {
  return operator()(std::forward<Args>(args)...); // by current implementation
  }

if you don't like to break the API of the argh::parser, add named functions in the first place and update your README and examples, then flag operators as deprecated.

p.s GSL has also some tips for operators.

[adishavit]

I've already fixed const correctness. Need to push it. Re: the rest will reply soon.

[adishavit]

Argh recognizes and demuxes the command line into, 3 different and independent argument types:

1. Positional,
2. Flag (boolean),
3. Parameter (Name + Value)

Each corresponds to a conceptually different container with different requirements:

1. Positional: a std::vector<std::string>
   • Sequential and accessed by index.
   • Returns input string
   • Provide convertibility to T
2. Flag: std::set<std::string> or a std::map<std::string,bool>
   • Can only be checked for existence (the string itself is meaningless, the user knows it)
   • (Support alternate names)
3. Parameter: std::map<std::string,std::string>
   • Checkable for existence
   • Returns value input string (param name is known to user)
   • Provide convertibility to T
   • (Provide optional default)
   • (Support alternate names)

Design Rationale

Argh supports these multiple intersecting requirements with a minimal set of (conceptually) 3 functions (if we ignore alt-names and defaults for a moment):

• Access by int means ordered/sequential, hence: Positional.
• Access by string means lookup in set/map.

So:

1. Access by [int] means get positional string, like argv[int].
2. Access by [string] means Flag, and thus, bool is the only return value that makes sense. Like std::map<std::string,bool>::operator[] it would return bool.
3. Access by (string) means Parameter, and thus, a map lookup. However, returning the value as a string, makes both bool and T conversion very verbose on the user side. So, yes, this is a departure from the STL-like access patterns.

Also:

• Access by (int) is a bonus function, that makes T convertibility easy.
• operator() also allow more than 1 argument, so we can easily support default values.

Design Considerations

Argh's design optimizes for:

• Conciseness:
  • Argh's effect on the user code should be as small as possible.
  • The user's code becomes shorter by using Argh.
• Minimalism: Do not allow ambiguity nor redundancy where none is required. This prevents redundant extra checking on the user side. So for example:
  • No getting flag string values.
  • Implicit conversion to bool

[These may not be a comprehensive list but will do for now]

Given these considerations, any additional named functions should retain the exiting principles.

So your suggested function at() for checking flags, what is the value you want to return? bool is a valid answer - so you get 2 overloads just like []. If you return a std::string then the user never needs its value and always needs to check .empty() which is unclear and verbose. Additionally, you would need an unambiguous way to check the existence of a named param. if(cmdl.at(name).empty()) is ambiguous (unclear of flag or param), and if(cmdl.to_stream(name)) does not read clearly.

If we need a named method, I am leaning towards has[] and has() for flags and parameters respectively (the name is from OpenCV's cli class). In fact, .has would be just decoration for readability and can be removed without changing the meaning of the code: if (cmdl["v"])... would be equivalent to: if (cmdl.has["v"])... I guess they could also be renamed to has_flag() and has_param().

Regarding to_stream() although that does not change anything, I'm not sure I see how it makes the code significantly more readable. Compare:

if (cmdl.to_stream(7))          // no sense here
   cmdl.to_stream(7) >> fname;  // Maybe, though it feels like the (7) is in the wrong place
cmdl.to_stream("t",128) >> threshold;

to:

if (cmdl(7))
   cmdl(7) >> fname; 
cmdl("t",128) >> threshold;

This is much shorter, and IMO just as clear if nor clearer. But this is a subjective matter though. I guess one needs to get used to the grammar of the lib.

One thing you mentioned is that it isn't possible to initialize a local variable with a non string cli value. You need to declare it first before you can stream into it. This bugged me because it goes against the principle of lowest-possible-impact on user code.

So, I am experimenting with a templated as<>() method (a-la Boost.Program_Options) to do this. The main problem here is that the internal streaming may fail and how to report this error.

• One option is to use exceptions only for this method (like Boost.Program_Options).
• std::option is not an option, since Argh is C++11 compatible,
• Another possibility, is to only provide the version of as with a default value:
  • These methods always had a potential for a streaming problem, but it is under the user's control.
  • If the streaming fails, we set the default value
  • It also means the user does not need to provide the template type, it can be deduced from the default value!

I'm still mulling this over.

Finally, I also like the minimalism of: has and as.

[azadkuh]

your design rationale about three categories of flags, positional and parameters looks familiar and comprehensible for every command line user!

you've already classified them as:

// parser:
std::multiset<std::string> const&          flags();
std::map<std::string, std::string> const&  params();
std::vector<std::string> const&            pos_args() const;

So your suggested function at() for checking flags, what is the value you want to return?

well, i didn't. i expect at() / operator[] to behave as std::vector::at(), std::map::at, ... is_set() and has() (as you suggested) are better options to check against flags.

imho:

• has() / is_set() only look into flags()
• operator[](size_t) / at(size_t) look into pos_args()
• operator[std::string const&] / at(std::string const&) look into params()

minimal changes

with minimal changes (by separating flags from operator[]/at()) the current design:

// operator[] overloads for pos_args / flags
auto action = cmdl[2]; // also may return an empty string
if (action.empty()) {}
if (cmdl["amend"]) {}

// operator() overloads for pos_args / params. may return bad_stream
auto stream = cmdl(2); // stream for positional arg
auto stream = cmdl("username"); // stream for parameter

turns into:

// operator[] or at() overloads for pos_args / params
auto action = cmdl[2];
auto user   = cmdl["username"];

if (cmdl.is_set("amend")) {}

there should be minimal surprise for argh users if they know stl or even Qt containers, and i guess it's not that ambiguous :)

hiding (or deprecating) operator() and providing a named function also helps, cmdl[]/cmdl() is the real ambiguity for someone who is not familiar with argh or code reviewers.

has_xxx()

i really like your idea about has_flag() and has_param(), eps the latter one that saves one single check against std::map::find() or ...

return values

the current design may also return an empty string or a bad stream (and i'm not against it). the user is supposed to check the return values:

// current design:

// given 2 positional args:
auto action = cmdl[7]; // action is empty

// given no argument as port:
uint16_t port{0};
cmdl("port") >> port; // a bad_stream, port is not set

exception safety

• which kind of exception safety does argh provide?
• it would be fine if you add noexcept to some methods: at least the parser::flags(), parser::params() and parser::pos_args()
• you may hate this one! :) but it's not that horrible if at() or operator[] throw anstd::out_of_range, as the internally used std::vector::at() and std::map::at() do.

as<>()

it is short and looks nice, but value<>() is just 3 characters longer and explicitly shows the intent :))