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

fix(cmd): fix logging of help message #395

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Conversation

asraa
Copy link
Contributor

@asraa asraa commented Sep 22, 2022

Signed-off-by: Asra Ali asraa@google.com

Fixes comments of #385

* fix(cmd): Prints help message on `Stdout` rather than `Stderr`

Please fill in the fields below to submit a pull request. The more information that is provided, the better.

Fixes #
Release Notes:

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfills the following requirements:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

Signed-off-by: Asra Ali <asraa@google.com>
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're nitpicking what goes where...I build tuf at this PR and I see:

cmd output stream correct
tuf usage stdout *
tuf badcommand error stderr
tuf --badarg usage stderr
tuf help help stderr
tuf --help usage stderr
tuf init --help help stdout
tuf init --badarg usage stdout

* I think this one's debatable: is the user explicitly asking for the usage information? IMO no, they've invoked tuf incorrectly (if tuf had a no-args mode, it could be a correct usage)

Can we fix those too?

@asraa
Copy link
Contributor Author

asraa commented Sep 27, 2022

Just looked into this further, since this was annoying to deal with:

BOTH the incorrect ./tuf Stdout help message is coming internally from the go-docopt library: https://github.com/flynn/go-docopt/blob/f6dd2ebbb31e9721c860cf1faf5c944aa73e3844/docopt.go#L55

that triggers when the user gives bad input: which either means no command here, or a bad input argument.

Any maintainer have any context why we're not using https://github.com/docopt/docopt.go? Was it too complex a dependency?
And there is no way for me to change the output stream

@mnm678
Copy link
Collaborator

mnm678 commented Sep 27, 2022

Any maintainer have any context why we're not using https://github.com/docopt/docopt.go? Was it too complex a dependency?

Looks like this decision was made in 2015 and not revisited (also, https://github.com/docopt/docopt.go was still pretty new in 2015). If a different library has features we need, I think we should switch.

@znewman01
Copy link
Contributor

Just looked into this further, since this was annoying to deal with:

Okay, I don't want to block this PR on you totally switching over our CLI engine.

Any maintainer have any context why we're not using https://github.com/docopt/docopt.go? Was it too complex a dependency?

This is all speculation/hearsay, but IIRC Flynn was a big go-tuf user/contributor so a long time ago we bought into their go stack. However Flynn is sadly unmaintained so a lot of that is relatively dated.

I think a switch to more mainstream dependencies in place of the Flynn ones would be a good idea.

@asraa
Copy link
Contributor Author

asraa commented Sep 27, 2022

I'll create a follow up good first issue: it would be great to move over to another managing library then

@trishankatdatadog trishankatdatadog merged commit f75cbcc into master Sep 27, 2022
@trishankatdatadog trishankatdatadog deleted the fix-log-usage branch September 27, 2022 17:40
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.

4 participants