-
Notifications
You must be signed in to change notification settings - Fork 109
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
feat: pass logger into repo and client #385
Conversation
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW nice refactor with the "Opts" pattern 👍
Signed-off-by: Asra Ali <[email protected]>
Signed-off-by: Asra Ali <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter is a nice touch
@@ -32,7 +33,7 @@ See "tuf-client help <command>" for more information on a specific command. | |||
|
|||
if cmd == "help" { | |||
if len(cmdArgs) == 0 { // `tuf-client help` | |||
fmt.Println(usage) | |||
fmt.Fprint(os.Stderr, usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is usage going to stderr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 I've seen it both ways. There are some tools that define an explicit, strict "everything that gets logged to stdout must be computer-readable" philosophy and apply that to help text too. Many tools do, of course, print usage on STDOUT.
See for instance this request for usage on STDERR. I particularly like the recommendation to follow picocli (just one example):
when the user specified invalid input, the error message and usage help should be printed to stderr, but when the user requests help with --help, the usage help should be printed to stdout so users can pipe the output to tools like grep and less.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you for the comment: Yeah, I think I originally assumed that help should be part of STDERR, but here, since the user explicitly requested for help, I think I concur, it should be STDOUT. I'll put out a fix PR.
@@ -58,7 +58,7 @@ See "tuf help <command>" for more information on a specific command | |||
|
|||
if cmd == "help" { | |||
if len(cmdArgs) == 0 { // `tuf help` | |||
fmt.Println(usage) | |||
fmt.Fprint(os.Stderr, usage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Signed-off-by: Asra Ali [email protected]
Fixes #376
Please fill in the fields below to submit a pull request. The more information that is provided, the better.
Release Notes:
Types of changes:
Description of the changes being introduced by the pull request:
Client and Repo can now pass in an optional logger. This will not break any existing clients, the argument is optional.
Please verify and check that the pull request fulfills the following requirements:
**This is pretty preliminary, please let me know if you agree/disagree with my approach.
I would still need to fix the CLI as a follow-up, this PR as it stands totally removes logging since none is explicitly passed.
**