Make Print action print to the proper io::Write object

[d-e-s-o]

The Print action prints data straight to stdout, which is not what users expect from a context where custom stdout and stderr io::Write objects can be supplied. This change fixes the problem. It piggy-backs on the Exit variant of the ParseResult enum and adds an optional String argument to it. This argument, if present, will be printed to the correct io::Write object that the user intended to represent stdout, just before exiting the program.

[tailhook]

Hi,

Is API change justified? Maybe it's enough to fix just the output stream?

[d-e-s-o]

Is API change justified?

I did not see another way and that's how it's done for errors as well, so that just seems consistent.

Maybe it's enough to fix just the output stream?

Can you please be a bit more specific what exactly you have in mind?

[tailhook]

Okay. So what I do see here:

1. To make this work we need to either pass stream to IFlagAction or change ParseResult (as you've done in your PR).
2. Both options require changing public API (ParseResult is a public type, IFlagAction is a non-sealed public trait)

This limitation is easy to overcome: just make a boolean flag and print everything after arguments are parsed.

So given the state of this library (it's in maintenance/no-new-features mode, not recommended for the new pojects -- use StructOpt), I think it's better to close this pull request and reject the feature. I think requiring everybody to upgrade the library to a non-backward-compatible one is not justified for this case.

What do you think?

[d-e-s-o]

Thanks for elaborating on your thoughts.

This limitation is easy to overcome: just make a boolean flag and print everything after arguments are parsed.

Is it that simply, though? Based on my arguably limited investigation (and knowledge of argparse) that does not work that easily (and feel free to correct me if I am missing something). We have subcommands in our program and those are required. Now with the Print action argparse would take care of stopping the parsing of arguments, printing, and exiting. With an argument as you suggest, however, it will complain that required arguments are missing.

So given the state of this library (it's in maintenance/no-new-features mode, not recommended for the new pojects -- use StructOpt), I think it's better to close this pull request and reject the feature.

I am surprised by this stance. For one: this is a bug fix. A bug fix that entails breaking compatibility (although in arguably a trivially fixable way) but still a bug fix. Or are we arguing that this behavior is intentional and correct in the face of the ArgumentParsers's parse method accepting custom io::Write objects?

I was not aware of the maintenance/new-no-features mode, and quite frankly, it's not communicated anywhere where people tend to look from what I can tell (i.e., in the README which pops up on crates.io and on Github). So if the above is your idea of the future of the project then I would like to ask you to communicate that appropriately.

I think requiring everybody to upgrade the library to a non-backward-compatible one is not justified for this case.

I don't see any requirement for people to update. If they do, that's not hard. If they don't, no harm done. They most likely want to eventually, but again, I don't expect too many people to be affected, and if they are the fix is trivial. It would be nice to communicate what changed, though, because that may not be obvious. So a changelog as proposed in the three year old pull request may be a nice-to-have :D

What do you think?

I would encourage you to rethink the role the crate plays. Allow me to provide my view: I don't think that StructOpt is necessarily a proper replacement. Yes, it probably is superior in pretty much any aspect concerning argument parsing. Yet, anybody caring about build time and binary size will carefully evaluate the usage of StructOpt and clap (the former depends on the latter and below I don't differentiate much; hopefully not for the worse), as they are pretty big and come with a good amount of dependencies. Last I checked (and I acknowledge that this was quite a while back [although after the author already trimmed it down) clap induced quite an increase in binary size. But I just tried it out again and here are the numbers:

With clap: 1194456 bytes Without clap: 625112 bytes

That is ridiculous, to say the least. Binary size (release + stripped) doubles just by virtue of adding clap (and that's already with default-features turned off [although that does seemingly not make a difference here]). It's not a completely fair comparison because I did not add a replacement (I still had this old testing branch lying around and just updated and repurposed it) but on a slightly more progressed state of the program where I used getopts (not argparse), the binary is still at 674264 bytes. That just goes to show that clap adds quite a bit of bloat.

All that goes to say that I think that argparse is uniquely positioned to fill a niche for folks that care about binary bloat and can live with the comparatively limited set of functionality, but do not want to get into the business of reimplementing their own argument parser. It has no dependencies and it does its job reasonably well. It has a good amount of tests giving confidence in the quality of the crate. It does support subcommands which crates such as getopts (and more) do not.

[tailhook]

Okay, you've made valid points. Let's rehash that a bit in #51 before deciding on what to do here.

[d-e-s-o]

Hey @tailhook , what's the story on this pull request? Can it be merged?

[tailhook]

I need more time. Even if it's merged right now I need more time before a release to review other pending changes to make breaking changes less frequent. In the meantime, you can probably use git version?

[d-e-s-o]

In the meantime, you can probably use git version?

The problem is that this bug fix is blocking a feature in nitrocli from being released. A new release of nitrocli needs a proper crates.io available version of argparse from what I understand.

[d-e-s-o]

Hi @tailhook,

I need more time. Even if it's merged right now I need more time before a release to review other pending changes to make breaking changes less frequent.

have you been able to get to some of the other changes? Do you have a rough timeline for the release?