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
fortio --help should go to stdout not stderr #130 #264
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @olimpias. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
I prefer to use help instead of --help because other arguments are done without dashes(like fortio load). But I can add if you want. |
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
======================================
Coverage 90.6% 90.6%
======================================
Files 10 10
Lines 1952 1952
======================================
Hits 1769 1769
Misses 117 117
Partials 66 66 Continue to review full report at Codecov.
|
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.
nice, thanks, can we do like version where both fortio version and fortio -version works (so -help and help to stdout)?
Sure. I will take a look tomorrow to them.
… On 26 Jun 2018, at 12:11 AM, Laurent Demailly ***@***.***> wrote:
@ldemailly commented on this pull request.
nice, thanks, can we do like version where both fortio version and fortio -version works (so -help and help to stdout)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
code/hack is in bin common, so maybe an issue to pass some call back. maybe not necessary/fine as is (should document “help” subcommand too) |
I think you are referring to |
I see your point common flag is all about flags. When I move |
My change on version printing is broke the release_test. I will take a look at night and reverse it. Nevermid I have done it now :). |
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.
see below,
also please test and paste the output of
fcurl
fcurl help
fortio
fortio help
fortio -help
bincommon/commonflags.go
Outdated
|
||
// Usage prints usage according to input writer | ||
func Usage(writer io.Writer) { | ||
fmt.Fprintf(writer, "Φορτίο %s usageErr:\n\t%s command [flags] target\n%s\n%s\n%s\n%s\n", |
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.
usage not usageErr here
Outputs; fortio fcurl and fortio fcurl help
fortio
fortio help and fortio -help
|
I did mean fcurl, not fortio curl the is the reason for bincommon, some flags are shared between those 2 seperate binaries |
fcurl
fcurl help
I didn't realize the fcurl binary :) |
fcurl help is wrong, doesn’t match the right short usage (there is no command in fcurl) |
I fixed it now and compared with master. fcurl
fcurl help
|
bincommon/commonflags.go
Outdated
version.Short(), | ||
os.Args[0], | ||
"where command is one of: load (load testing), server (starts grpc ping and", | ||
"http echo/ui/redirect/proxy servers), grpcping (grpc client), report (report only UI", |
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.
leave that part in fortio_main as it is specific to the fortio binary (and fcurl can have its own specific string)
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 wrote that yesterday but somehow it didn't post ^)
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 was using Usage
to print for help command in SharedMain method. Do you have any suggestion how I can move Usage
method to fortio_main? If I move it, I will need to move help command action to fortio_main as well.
I am doing the change needed on your PR directly, upcoming PR update |
and use new command to simplify the release/updateFlags.sh script - no more need to skip last line / error message
Linter are useful this was a real bug in my change (We should ideally have a test for the command line errors)
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.
approving my own changes :-)
Thanks! I see what you meant. Pass function pointer of usage from fortio_main and fcurl to sharedMain method to callback. I couldn't think like that. I need to get used to pass function pointer :). |
yes - it's a bit convoluted - we could also have passed a format/help string but this was overall minimizing the diff (also in go shorter names |
help is handled as an input argument and usage message is divided into two as info and error.