Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp: Add exitcode::USAGE exit code as suggested in #1327 #1637

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

danielthegray
Copy link
Contributor

The reason to do this is described well in issue #1327.
It is also a recommendation of the Rust book itself:
https://rust-cli.github.io/book/in-depth/exit-code.html

I have run the tests with no failures:

$ cargo test --no-default-features
...
test result: ok. 286 passed; 0 failed; 11 ignored; 0 measured; 0 filtered out
$ cargo test --features "yaml unstable"
...
test result: ok. 286 passed; 0 failed; 14 ignored; 0 measured; 0 filtered out

The reason to do this is described well in issue clap-rs#1327. It is also a
recommendation of the Rust book itself:
https://rust-cli.github.io/book/in-depth/exit-code.html
@pksunkara pksunkara changed the title imp: Add exitcode::USAGE exit code as suggested in #1327 Merge pull request #1637 from danielthegray/use-USAGE-error-exit-code Jan 30, 2020
@pksunkara pksunkara merged commit e1ae505 into clap-rs:master Jan 30, 2020
@danielthegray
Copy link
Contributor Author

danielthegray commented Jan 31, 2020

Thank you for merging it! I think there are some spots where the documentation needs to be updated as well.

EDIT: I've looked and can't seem to find the place I saw earlier. Anyway, thanks a lot!

@pksunkara pksunkara changed the title Merge pull request #1637 from danielthegray/use-USAGE-error-exit-code imp: Add exitcode::USAGE exit code as suggested in #1327 Feb 1, 2020
@BurntSushi
Copy link
Contributor

Could the addition of a new dependency here be reconsidered? This is adding a non-optional crate dependency just to use a single number. These numbers are pretty static, so it'd be just fine to define const USAGE: ExitCode = 64; internally IMO.

Moreover, exitcode itself really isn't giving you much here. Arguably, if exit codes differed by platform, then exitcode could be doing the laborious work of picking the correct exit code for you given the use case and platform. But it doesn't do that. Internally, there are no platform specific definitions. On Windows, it seems that there are no strong conventions to follow.

On Unix, it seems like many applications don't bother with this at all. On my Linux system with GNU ls:

$ ls --not-an-option
ls: unrecognized option '--not-an-option'
Try 'ls --help' for more information.
$ echo $?
2

On macOS:

$ ls --not-an-option
ls: illegal option -- -
usage: ls [-@ABCFGHLOPRSTUWabcdefghiklmnopqrstuwx1%] [file ...]
$ echo $?
1

Indeed, it's not actually clear to me that sysexits.h is actually standard. In most cases I'm aware of, a CLI usage error results in an exit code of 2, not 64.

So I guess I've muddled two concerns here:

  1. Adding a new non-optional dependency that IMO does not carry its weight. This adds a totally new project that dependents of clap now also need to audit.
  2. It is not clear to me that the behavior here is actually ideal. I'd rather that clap conform to established custom rather than well meaning attempts to make exit codes more meaningful.

@Dylan-DPC-zz
Copy link

I agree with @BurntSushi here. Clap is already a big project that adding a new dependency should be only in required justifiable cases

@pksunkara
Copy link
Member

Yup, my bad. I thought the exit codes differed on windows.

bors bot added a commit that referenced this pull request Feb 3, 2020
1653: Revert " imp: Add exitcode::USAGE exit code as suggested in #1327" r=pksunkara a=Dylan-DPC

Reverts #1637

Co-authored-by: Dylan DPC <dylan.dpc@gmail.com>
Co-authored-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
bors bot added a commit that referenced this pull request Feb 3, 2020
1653: Revert " imp: Add exitcode::USAGE exit code as suggested in #1327" r=me a=Dylan-DPC

Reverts #1637

Co-authored-by: Dylan DPC <dylan.dpc@gmail.com>
Co-authored-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
epage added a commit to epage/clap that referenced this pull request Jul 19, 2021
PR clap-rs#1637 switched clap to report `64` on errors and then clap-rs#1653 switch it
to 2, but both missed a case.  This also documents the reason why inline
since I had to go and dig through the history to re-discover the
motivation.
epage added a commit to epage/clap that referenced this pull request Jul 19, 2021
PR clap-rs#1637 switched clap to report `64` on errors and then clap-rs#1653 switch it
to `2`, but both missed a case.  This also documents the reason why inline
since I had to go and dig through the history to re-discover the
motivation.
epage added a commit to epage/clap that referenced this pull request Jul 19, 2021
PR clap-rs#1637 switched clap to report `64` on errors and then clap-rs#1653 switch it
to `2`, but both missed a case.  This also documents the reason why inline
since I had to go and dig through the history to re-discover the
motivation.
epage added a commit to epage/clap that referenced this pull request Jul 19, 2021
PR clap-rs#1637 switched clap to report `64` on errors and then clap-rs#1653 switch it
to `2`, but both missed a case.  This also documents the reason why inline
since I had to go and dig through the history to re-discover the
motivation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants