[FEATURE] Automatically open man page

[eseiler]

#170

Questions:

• Should this be default?
• Can this be tested? If untestable, it probably shouldn't be default.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
sharg-parser	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 21, 2023 at 8:40PM (UTC)

[codecov]

Codecov Report

Base: 92.29% // Head: 91.58% // Decreases project coverage by -0.72% ⚠️

Coverage data is based on head (59c5575) compared to base (9ba04a4).
Patch coverage: 60.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #173      +/-   ##
==========================================
- Coverage   92.29%   91.58%   -0.72%     
==========================================
  Files          16       16              
  Lines        1440     1461      +21     
==========================================
+ Hits         1329     1338       +9     
- Misses        111      123      +12     

Impacted Files	Coverage Δ
include/sharg/detail/format_base.hpp	92.93% <ø> (-0.04%)	⬇️
include/sharg/detail/version_check.hpp	52.80% <ø> (ø)
include/sharg/detail/format_man.hpp	81.96% <33.33%> (-15.95%)	⬇️
include/sharg/parser.hpp	98.66% <77.77%> (-0.88%)	⬇️
include/sharg/detail/format_help.hpp	89.56% <100.00%> (ø)
include/sharg/detail/terminal.hpp	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[h-2]

Thanks for the quick PR!

If you don't want to turn this on by default, can you add a nob in the parser, maybe just a bool help_as_man = false; that I can set to true in my programs?

[eseiler]

So, like parser.help_as_man = true;?

Another option could be to do to it via a definition SHARG_OPEN_HELP_AS_MAN, and then some #if defined.

[h-2]

So, like parser.help_as_man = true;?

yep, very straightforward and easy!

Another option could be to do to it via a definition SHARG_OPEN_HELP_AS_MAN, and then some #if defined.

Warum einfach, wenns auch kompliziert geht? 🙃

[eseiler]

Warum einfach, wenns auch kompliziert geht? 🙃

I'll need to wrap everything in coverage-excludes anyway :D
And you could change the behavior without touching the code

[h-2]

And you could change the behavior without touching the code

But with touching the preprocessor... Depending on build-system, this may or may not be easy. In any case, it is something I would only recommend as last resort. If a simple bool does it, why not use that? 🙂

In any case, thanks for working on this!

[eseiler]

The problem with the parser.help_as_man = true; approach is that the format is decided on construction. So, setting help_as_man after construction does not have any effect.

There could be a second constructor that takes another argument. I wouldn't add a defaulted argument to the existing constructor because it would be hard to document what part is experimental and what part stable.

[h-2]

The problem with the parser.help_as_man = true; approach is that the format is decided on construction. So, setting help_as_man after construction does not have any effect.

Ah, I thought that all the logic happens on parse(). That's unfortunate.

There could be a second constructor that takes another argument. I wouldn't add a defaulted argument to the existing constructor because it would be hard to document what part is experimental and what part stable.

Hm, yeah, having multiple, party defaulted arguments scales very badly. A config object like the other ones would have likely been better. Maybe that could be added with an extra constructor? Then you can also document the members of the struct individually.

[eseiler]

I think adding a config would be quite a big change to for implementing this feature.
I'm not certain how nice it would be to have a config for the constructor, because (I assume) most people just do sharg::parser parser{"Name", argc, argv};.

So, right now, I'd say we have two possibilities:

Changing the constructor

It can't be an additional constructor (as long as we use the same arguments) because it would be ambiguous.

    parser(std::string const & app_name,
           int const argc,
           char const * const * const argv,
           update_notifications version_updates = update_notifications::on,
           std::vector<std::string> subcommands = {},
           open_help_as_man = open_help_as_man::off)

Adding a definition

For example, via a CMake option. Something like -DSHARG_OPEN_HELP_AS_MAN.
The default could be undefined. Doesn't make too much difference concerning the lines of code because I would need to add // GCOVR_EXCL_{LINE,START,STOP} for the coverage test.

Depending on how you look at it, it might be considered an overall (dis-)advantage to set it via the build system/compiler definition. The disadvantage is that doing it this way is a bit obscure. The advantage is that whoever builds an application can decide for themselves whether they want this behavior, without actually changing any code. In the other case, it would be a developer decision, like setting update_notifications::off in the parser constructor.
I am not certain how handy this would be for, e.g., package maintainers. Personally, I think it would be nice to toggle this behavior without having git showing changes or being forced to accept whatever the developer decided.

P.S.: We have a paper review deadline on March 16th, so we might be slower than usual :)

[h-2]

Thank you for your work on this and the detailed comments. I was unaware that relevant logic (format selection) happens at construction. I don't know why it couldn't happen later? But I am sure there were good reasons for this. ¹

At the current state, I see three options, in descending preference:

1. Just change the default.
2. Add another constructor that takes a config.
3. Add another defaulted argument to the constructor.

Personally, I think 1. is great, and people will love it; it's a cool feature that everyone will experience immediately. It will not break any code, because any non-interactive use of the parser will trigger the old behaviour anyway. But I understand if you are reluctant about this.

The second option looks like a bit more work, but I think it's actually a good future-proof change. I would do it as parser p{"git", args, config}, because the first two args are required. The only problem I see, is that sharg::config as a name is already in use for the option-config. This is not a huge problem, since you can do parser p{"git", args, {.foo = 1}} without knowing the name of the config type, but it is still something to consider.

The third option is more of a band-aid, but something I could live with.

Using a C macro is very undesirable from my point of view. It needs to be set before including the header, so the configuration of the parser becomes very "unlocal". A CMake option would be even worse, because then I need to move the configuration out of my C++ source code.

Thank you in any case!

¹ On a very general note, maybe for a future version: I think it would have been preferable to separate "setup" of the parser (how it works, what it looks for) and "applying" the parser to user-provided arguments (i.e. using the parser). One way to do this would be to provide the argv to the .parse() function instead of the constructor. But this is Off-topic here.

[smehringer]

I'm in favor of the config object because

1. Changing the default means that the default is something untestable (or really weirdly testable).
2. A CMake variable is harder to set.
3. The config object is more future proof than another defaulted constructor argument.
4. A bolean parser.help_page_as_man = true is inconsistent as all other configurations are done at construction. (It would be possible by the way with some internal rearrangements)

While thinking about this, I propose the following constructor:

sharg::parser p{argc, 
                argv, 
                sharg::parser::config{.notify_about_updates = true, 
                                      .subcommands = {"foo"},
                                      .interactive_man_page = true}};

note:

In my proposal the app name is not part of the constructor anymore. I notices that we can (currently!) also set the app name via parser.info.app_name. That suffices IMHO and having both is (a) confusing and (b) buggy in our code, see #182.

This c-tor change could/would be a minor release, deprecating the other constructor. We could also think about a major release right away.

[eseiler]

I'm in favor of the config object because

👍

This probably means that we will add the config in another PR, and this PR then adds a new config element and implements the functionality.

[h-2]

While thinking about this, I propose the following constructor:

This looks very good!

[eseiler]

also do the same for cerr