Provide more helpful error message and require input for every non-bool option. #519

wyk9787 · 2020-04-08T21:57:45Z

Fixes: #441

More helpful error messages and usages are added.
All non-bool option now requires an input. That means "call_tree" is valid, but "sample_index" is invalid.

Before:

(pprof) sample_index
(pprof)

(pprof) sample_index 0
unrecognized command: "sample_index"

(pprof) help
    ...
    sample_index     Sample value to report (0-based index or name)
    ...

After:

(pprof) sample_index
please input a value, e.g. sample_index=<val>
(pprof) focus
please input a value, e.g. focus=<val>
(pprof) call_tree
(pprof)

(pprof) sample_index 0
do you mean: "sample_index=0"?

…ol option.

codecov-io · 2020-04-08T22:37:16Z

Codecov Report

Merging #519 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   67.06%   67.11%   +0.04%     
==========================================
  Files          37       37              
  Lines        7622     7627       +5     
==========================================
+ Hits         5112     5119       +7     
+ Misses       2104     2103       -1     
+ Partials      406      405       -1

Impacted Files	Coverage Δ
internal/driver/interactive.go	`61.65% <100.00%> (+1.50%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1a9688...fc94335. Read the comment docs.

aalexand · 2020-04-08T22:51:51Z

Could you include "before" and "after" outputs in the PR description? Basically, enough data for the reviewer to convince themselves that the bug is fixed on all its points.

nolanmar511

Awesome start! Can't wait for this change to get in.

This is just a bookkeeping type nit: Could you mention the issue this is addressing in the PR description?

If a PR fully fixes an issue, adding something like "Fixes #" to the PR description will close the issue automatically when the PR is merged. If the PR only address part of an issue, it's nice to mention if there are next steps.

internal/driver/commands.go

internal/driver/interactive.go

internal/driver/commands.go

internal/driver/interactive_test.go

…_error

aalexand · 2020-04-09T17:43:07Z

On

(pprof) sample_index 0
Unrecognized command: "sample_index"

vs.

(pprof) sample_index
please input a value, e.g. option = value

(pprof) focus
please input a value, e.g. option = value

(pprof) sample_index 0
do you mean: "sample_index=0"?

Casing is inconsistent. User visible messages should use sentence case.

internal/driver/commands.go

wyk9787 · 2020-04-09T17:53:36Z

On

(pprof) sample_index 0
Unrecognized command: "sample_index"

vs.

(pprof) sample_index
please input a value, e.g. option = value

(pprof) focus
please input a value, e.g. option = value

(pprof) sample_index 0
do you mean: "sample_index=0"?

Casing is inconsistent. User visible messages should use sentence case.

Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings

aalexand · 2020-04-09T17:56:27Z

On
(pprof) sample_index 0
Unrecognized command: "sample_index"
vs.
(pprof) sample_index
please input a value, e.g. option = value

(pprof) focus
please input a value, e.g. option = value

(pprof) sample_index 0
do you mean: "sample_index=0"?
Casing is inconsistent. User visible messages should use sentence case.
Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@wyk9787 I took the example from the PR description. It's fine to keep these lowercase, but the casing should be consistent.

wyk9787 · 2020-04-09T18:01:24Z

On
(pprof) sample_index 0
Unrecognized command: "sample_index"
vs.
(pprof) sample_index
please input a value, e.g. option = value

(pprof) focus
please input a value, e.g. option = value

(pprof) sample_index 0
do you mean: "sample_index=0"?
Casing is inconsistent. User visible messages should use sentence case.
Actually before was lowercase as well and was consistent throughout the code base, i.e. "unrecognized command ...". Also, my IDE reminds me that error message should not start with uppercase, https://github.com/golang/go/wiki/CodeReviewComments#error-strings
@wyk9787 I took the example from the PR description. It's fine to keep these lowercase, but the casing should be consistent.

@aalexand Sorry about! Description is updated.

internal/driver/interactive.go

nolanmar511

LGTM overall, with a few nits.

Could you also include the output for pprof -help in the description?
I'd like to be able to check that both interactive and non-interactive help modes produce consistent results.

internal/driver/commands.go

nolanmar511

Still LGTM overall.

internal/driver/commands.go

aalexand · 2020-04-10T18:13:01Z

internal/driver/commands.go

@@ -246,6 +246,22 @@ func helpText(s ...string) string {
 	return strings.Join(s, "\n") + "\n"
 }

+func variablesPostFix(name string, vr *variable) string {


The name of this function is unclear. I don't understand what it's doing from the name.

aalexand · 2020-04-10T18:13:24Z

internal/driver/commands.go

@@ -254,7 +270,7 @@ func usage(commandLine bool) string {
 		prefix = "-"
 	}
 	fmtHelp := func(c, d string) string {
-		return fmt.Sprintf("    %-16s %s", c, strings.SplitN(d, "\n", 2)[0])
+		return fmt.Sprintf("    %-22s %s", c, strings.SplitN(d, "\n", 2)[0])


Curious: why did 16 change to 22?

To accommodate extra texts (e.g. =) added to the help message and really long option name (i.e. relative_percentages)

aalexand · 2020-04-10T18:14:16Z

internal/driver/commands.go

-		"  Options:\n"
+		"  Options:\n" +
+		"    General format is <option>=<value>\n" +
+		"    <f> is a float, <n> is an integer, and <s> is a string\n\n"


What about booleans? E.g. how to set a boolean to false?

wyk9787 · 2020-04-13T20:17:43Z

Reverted all modifications to the help text and updated description.

nolanmar511

LGTM overall -- a few nits.

nolanmar511 · 2020-04-14T15:15:51Z

internal/driver/interactive.go

@@ -70,6 +70,11 @@ func interactive(p *profile.Profile, o *plugin.Options) error {
 					value = strings.TrimSpace(value)
 				}
 				if v := pprofVariables[name]; v != nil {
+					// All non-bool options require inputs
+					if v.kind != boolKind && value == "" {
+						o.UI.PrintErr(fmt.Errorf("please input a value, e.g. %v", name+"=<val>"))


Nit:
Here, I think %s would be more idiomatic than %v.
The following would be standard for Golang:

o.UI.PrintErr(fmt.Errorf("please input a value, e.g. %s=<val>", name))

Done.
Really appreciate those comments to help me write better Go code. Thanks!

nolanmar511 · 2020-04-14T15:18:44Z

internal/driver/interactive.go

@@ -267,6 +272,9 @@ func parseCommandLine(input []string) ([]string, variables, error) {
 		}
 	}
 	if c == nil {
+		if v := pprofVariables[name]; v != nil {
+			return nil, nil, fmt.Errorf(`did you mean: %s?`, name+"="+args[0])


Nit: Same as above.
Also nit: I don't think back ticks have added benefit here, so I'd reccomend quotes,
So, following is more idiomatic (assuming args is a slice of strings):

fmt.Errorf("did you mean: %s=%s", name, args[0])

nolanmar511 · 2020-04-14T15:30:17Z

internal/driver/interactive_test.go

+		{"Random interleave of independent scripts 2", interleave(script, 1), pprofShortcuts, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 1", scScript1, shortcuts1, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 2", scScript2, shortcuts2, "", 0, false},
+		{"Group with invalid vale", []string{"cumulative=this"}, pprofShortcuts, `unrecognized value for cumulative: "this". Use one of cum, flat`, 1, false},


(super nit) I think this is a typo: vale => value

nolanmar511 · 2020-04-14T15:38:24Z

internal/driver/interactive_test.go

-		t.Error("expected IO error, got nil")
+			// Confirm error message written out once.
+			if tc.numAllowRxMatches != ui.NumAllowRxMatches {
+				t.Errorf("want error message to be printed %v time(s), got %v",


Use %d instead of %v?
(I know this line already existed, but nice to be able to make it a bit more idiomatic.)

nolanmar511 · 2020-04-14T15:39:08Z

internal/driver/interactive_test.go

-	}
+			err := interactive(p, o)
+			if (tc.propagateError && err == nil) || (tc.propagateError && err != nil) {
+				t.Errorf("%v: %v", tc.name, err)


Nit: use %s when possible

t.Errorf("%s: %v", tc.name, err)

aalexand · 2020-04-15T17:05:17Z

Or, I should simply discard those changes for the help in this PR?

I would probably leave out that part of changing the help message. (And revert the header of the options section as needed)

wyk9787 · 2020-04-15T17:09:56Z

Or, I should simply discard those changes for the help in this PR?

I would probably leave out that part of changing the help message. (And revert the header of the options section as needed)

Yes, already reverted all changes made to the help message. There is no modification to the help message in this PR and the description is updated as well.

aalexand · 2020-04-16T15:56:43Z

internal/driver/interactive_test.go

+		shortcuts         shortcuts
+		allowRx           string
+		numAllowRxMatches int
+		propagateError    bool


This flag seems to be false in all test cases, is this expected?

Now fixed.
The conditional logic on line 79 was incorrect and is fixed now.

aalexand · 2020-04-16T18:06:19Z

internal/driver/interactive.go

@@ -70,6 +70,11 @@ func interactive(p *profile.Profile, o *plugin.Options) error {
 					value = strings.TrimSpace(value)
 				}
 				if v := pprofVariables[name]; v != nil {
+					// All non-bool options require inputs
+					if v.kind != boolKind && value == "" {
+						o.UI.PrintErr(fmt.Errorf("please input a value, e.g. %s=<val>", name))


input -> specify

aalexand · 2020-04-16T19:34:49Z

@wyk9787 The travis build is failing?

wyk9787 · 2020-04-16T19:57:20Z

@wyk9787 The travis build is failing?

It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.

Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.

nolanmar511 · 2020-04-16T20:08:14Z

@wyk9787 The travis build is failing?

It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.

Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.

For now, I restarted the errored tests. Can other folks restart these tests, too?

wyk9787 · 2020-04-16T20:12:12Z

@wyk9787 The travis build is failing?

It seems like the Travis times out before the test is even run: https://travis-ci.org/github/google/pprof/jobs/675881270.
Is it possible to increase the time limit for travis? Currently it's set to 50 minutes.

For now, I restarted the errored tests. Can other folks restart these tests, too?

I don't think I am able to.

wyk9787 · 2020-04-17T00:04:55Z

@aalexand The Travis build just passed.

Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504

* Update pprof to latest revision Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504 * Update P/pprof/build_tarballs.jl - use a real version number Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Remove now unused `timestamp` * [pprof] Use `GitSource` Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

…ol option. (google#519) Provide more helpful error message and require input for every non-bool option.

Provide more helpful error message and require input for every non-bo…

a9ea908

…ol option.

googlebot added the cla: yes label Apr 8, 2020

kalyanac requested a review from nolanmar511 April 8, 2020 22:06

Merge branch 'master' into helpful_error

2a191ba

nolanmar511 reviewed Apr 8, 2020

View reviewed changes

wyk9787 added 4 commits April 8, 2020 17:19

Improve the Option groups' help message

a2e56b4

Rearrange TestShell to table driven tests

f3c38ed

Merge branch 'helpful_error' of github.com:wyk9787/pprof into helpful…

8563fb1

…_error

Remove unused function

99ff77d

Remove space around equal sign in usage

b6459a5

nolanmar511 reviewed Apr 9, 2020

View reviewed changes

internal/driver/commands.go Outdated Show resolved Hide resolved

nolanmar511 reviewed Apr 9, 2020

View reviewed changes

internal/driver/interactive.go Outdated Show resolved Hide resolved

wyk9787 added 2 commits April 9, 2020 11:42

Standardize the help message for all options

7483e19

Fix tests

a0929fd

nolanmar511 reviewed Apr 9, 2020

View reviewed changes

internal/driver/commands.go Outdated Show resolved Hide resolved

internal/driver/commands.go Outdated Show resolved Hide resolved

internal/driver/commands.go Outdated Show resolved Hide resolved

wyk9787 added 2 commits April 9, 2020 17:15

Mention option <s> in help

2434176

Rename the function from addVariablesPostFix to variablesPostFix

68e3bcf

nolanmar511 reviewed Apr 10, 2020

View reviewed changes

internal/driver/commands.go Outdated Show resolved Hide resolved

Improve the help message on individual option

fee78be

nolanmar511 previously approved these changes Apr 10, 2020

View reviewed changes

wyk9787 mentioned this pull request Apr 10, 2020

Support Intel syntax in the assembly reports #520

Merged

aalexand reviewed Apr 10, 2020

View reviewed changes

wyk9787 added 2 commits April 13, 2020 13:13

Fix merge conflicts

cebe0e4

Solve merge conflicts

cbe760c

nolanmar511 reviewed Apr 14, 2020

View reviewed changes

wyk9787 added 2 commits April 14, 2020 12:12

Change format string to be more idomatic

091f3d9

Fix typo

05e747c

nolanmar511 previously approved these changes Apr 14, 2020

View reviewed changes

wyk9787 closed this Apr 15, 2020

wyk9787 deleted the helpful_error branch April 15, 2020 01:15

wyk9787 restored the helpful_error branch April 15, 2020 01:15

wyk9787 reopened this Apr 15, 2020

aalexand reviewed Apr 16, 2020

View reviewed changes

Fix flags

cfc1286

wyk9787 dismissed nolanmar511’s stale review via cfc1286 April 16, 2020 17:42

aalexand reviewed Apr 16, 2020

View reviewed changes

Fix wording

fc94335

aalexand approved these changes Apr 17, 2020

View reviewed changes

aalexand merged commit c6e0a84 into google:master Apr 17, 2020

wyk9787 deleted the helpful_error branch April 30, 2020 17:02

NHDaly mentioned this pull request Nov 13, 2020

Update pprof to latest revision JuliaPackaging/Yggdrasil#2096

Merged

gmarin13 pushed a commit to gmarin13/pprof that referenced this pull request Dec 17, 2020

Provide more helpful error message and require input for every non-bo…

8275f50

…ol option. (google#519) Provide more helpful error message and require input for every non-bool option.

Provide more helpful error message and require input for every non-bool option. #519

Provide more helpful error message and require input for every non-bool option. #519

Uh oh!

Conversation

wyk9787 commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aalexand commented Apr 8, 2020

Uh oh!

nolanmar511 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aalexand commented Apr 9, 2020

Uh oh!

Uh oh!

wyk9787 commented Apr 9, 2020

Uh oh!

aalexand commented Apr 9, 2020

Uh oh!

wyk9787 commented Apr 9, 2020

Uh oh!

Uh oh!

nolanmar511 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolanmar511 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wyk9787 commented Apr 13, 2020

Uh oh!

nolanmar511 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aalexand commented Apr 15, 2020

wyk9787 commented Apr 8, 2020 •

edited

Loading

codecov-io commented Apr 8, 2020 •

edited

Loading