Dep status -f doc and string display #1549

zkry · 2018-01-20T19:29:45Z

What does this do / why do we need it?

This pul requests chages the structure of the data fed into the template used by the go status -f command. The BasicStatus struct had fields that were not directly of type string making certain template constructs confusing (ex. dep status -f='{{if eq .Constraint "master"}}{{.ProjectRoot}}{{end}}' doesn't work because .Constraint is of type gps.Constraint, not a string).

This PR takes such elements and puts them into a structure whos type is string making the above example work.

In addition, I added some documentation to the help prompt explainging the usable field names and giving an example.

What should your reviewer look out for in this PR?

Is there any other place / a better place that I should document the -f flag?

Which issue(s) does this PR fix?

fixes #1497
fixes #1540

darkowlzz

This looks good to me.

Let's add tests for the changes in templateOutput.BasicLine checking the rendered output.

Also, let's move the example with conditional template under -examples output, similar to dep ensure -examples, and have a simple template example in status help.

darkowlzz · 2018-01-21T11:51:46Z

cmd/dep/status.go

@@ -40,6 +40,15 @@ print an extended status output for each dependency of the project.
  TODO    Another column description
  FOOBAR  Another column description

+You may use the -f flag to create a custom format for the output of the
+dep status command. The available flags you can utilize are as follows:


flags -> fields

zkry · 2018-01-21T13:24:11Z

Sounds good. I'll get those changes in.

zkry · 2018-01-22T16:35:57Z

I turned out that there were some things I missed in the initial commit:

The program would panic if Constraint, Version, or Latest were nil. To fix this, I called the getConsolidatedXxx method for that property which does a nil check.
The template would behave strangely for various HTML related symbols. For example a less-than symbol in the constraint would display as < in the output. To address these issues I changed the imported package from http/template to text/template.

I also included the -example flag with some basic examples. I can change the examples/display for that if there's something off.

darkowlzz

The tests look good with a slight enhancement mentioned inline.

I would wait for another review for the content of help and -examples. Maybe we can shorten it.

Moving from html/template to text/template is fine. I never realized that we were using html and not text. Maybe the IDE decided to insert html version and it didn't get much attention.

darkowlzz · 2018-01-23T12:52:54Z

cmd/dep/status_test.go

@@ -150,6 +157,19 @@ func TestBasicLine(t *testing.T) {
 					t.Errorf("Did not find expected Table status: \n\t(GOT) %v \n\t(WNT) %v", buf.String(), wantStatus)
 				}
 			}
+
+			buf.Reset()
+			template, _ := template.New("status").Parse("->{{.Revision}}~{{.Constraint}}~{{.Version}}<-")


Let's have all the available template fields here so that the test fails if anything stops rendering.

zkry · 2018-01-23T13:14:39Z

Sounds good. I'll get that fix in. I'll also take the example feature out, create it's own issue and pull request for review.

darkowlzz · 2018-01-23T13:21:49Z

@zkry let the example be there for now and wait for another review. Also, since we try to keep dep similar to go, we might do something similar to what go list's help has for template.

zkry · 2018-01-23T13:26:25Z

Ok, I see now. That would be good to check.

zevdg · 2018-01-24T21:37:16Z

cmd/dep/status.go

+	Display the list of package names constrained on the master branch.
+	The -f flag allows you to use Go templates along with it's various 
+	constructs for formating the output data. See -help for available 
+	variables for this flag.


Small UX concern: I'd re-list the available variables here instead of making the user run a separate command. For most people trying to use this feature, I expect they'd want to see the example and the options side-by-side.

To keep it nice and DRY, a simple const templateVariablesLine = "ProjectRoot, Constraint, Version, Revision, Latest, and PackageCount.\n" could be used in both places.

I guess if we can put in a line of description telling the user where to go to see the variable names, we can instead add a line telling what those actually are. 😅

zevdg

Full disclosure: I'm not a dep maintainer, but CONTRIBUTING.md isn't clear on if reviewers from the community are appreciated. Since I had already read the patch and @darkowlzz asked for more review, I figured I'd try to help.

zevdg · 2018-01-25T21:27:53Z

cmd/dep/status_test.go

@@ -150,6 +157,19 @@ func TestBasicLine(t *testing.T) {
 					t.Errorf("Did not find expected Table status: \n\t(GOT) %v \n\t(WNT) %v", buf.String(), wantStatus)
 				}
 			}
+
+			buf.Reset()
+			template, _ := template.New("status").Parse("PR:{{.ProjectRoot}}, Const:{{.Constraint}}, Ver:{{.Version}}, Rev:{{.Revision}}, Lat:{{.Latest}}, PkgCt:{{.PackageCount}}")


IMO, Moving this template string closer to expected output (like right above the tests table definition) would help readability.

zevdg · 2018-01-25T22:03:24Z

cmd/dep/status.go

+	data := struct {
+		ProjectRoot, Constraint, Version, Revision, Latest string
+		PackageCount                                       int
+	}{


Any reason not to reuse rawStatus here?

I thought of that too actually. It probably would make more sense to use it instead of creating an anonymous struct.

zevdg · 2018-01-25T22:08:54Z

cmd/dep/status_test.go

+				if ok := strings.Contains(buf.String(), wantStatus); !ok {
+					t.Errorf("Did not find expected template status: \n\t(GOT) %v \n\t(WNT) %v", buf.String(), wantStatus)
+				}
+			}


This test is good, but it doesn't actually ensure that these types remain of type string, or more importantly that templates using the eq function continue to work which is sorta what this whole patch is about.

I'd recommend having another test (or another chunk in this test) that attempts equality on every field. This may not even need an expected output, but it should at least ensure that the template can be executed without errors.

darkowlzz · 2018-02-05T16:41:43Z

@zkry Commit 4b3ad2c added the dep binary. I don't think it's a good idea to let a binary increase the size of git repo. You can edit or squash that commit and remove the binary.

zkry · 2018-02-06T05:40:27Z

Thanks for pointing that out. I forgot about that.

zevdg

Just fix the merge conflict with 57f0a59 and this LGTM

zkry · 2018-02-07T06:02:45Z

👍 Ok, its fixed. It should be good now.

sdboyer

seems fine, just needs rebase

zkry · 2018-02-26T19:35:34Z

@sdboyer all good now :)

darkowlzz

LGTM!

Thanks a lot for working on this.

Update Changelog for #1549 and typo fixes

zkry requested a review from darkowlzz as a code owner January 20, 2018 19:29

googlebot added the cla: yes label Jan 20, 2018

zkry changed the title ~~Dep status f doc and string display~~ Dep status -f doc and string display Jan 20, 2018

darkowlzz reviewed Jan 21, 2018

View reviewed changes

darkowlzz added the area: status label Jan 21, 2018

darkowlzz reviewed Jan 23, 2018

View reviewed changes

zevdg reviewed Jan 24, 2018

View reviewed changes

zkry requested a review from sdboyer as a code owner January 25, 2018 19:56

zevdg suggested changes Jan 25, 2018

View reviewed changes

zkry force-pushed the dep-status-f-doc-and-string-display branch from 1b53890 to c4b6589 Compare February 6, 2018 05:43

zevdg approved these changes Feb 6, 2018

View reviewed changes

sdboyer approved these changes Feb 26, 2018

View reviewed changes

zkry added 4 commits February 26, 2018 22:27

Add description for the -f flag and make template data fields string

2784fec

Change help text. Add example text. Add test cases. Fix template bugs

30ad0f7

Update tests

87566fd

Squash the binary.

a3d5085

zkry force-pushed the dep-status-f-doc-and-string-display branch from c4b6589 to a3d5085 Compare February 26, 2018 19:31

darkowlzz approved these changes Mar 4, 2018

View reviewed changes

darkowlzz merged commit 68b3f93 into golang:master Mar 4, 2018

darkowlzz added a commit to darkowlzz/dep that referenced this pull request Mar 4, 2018

Update Changelog for golang#1549 and typo fixes

508372a

darkowlzz added a commit that referenced this pull request Mar 4, 2018

Merge pull request #1732 from darkowlzz/changelog-1549

8ba7a56

Update Changelog for #1549 and typo fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dep status -f doc and string display #1549

Dep status -f doc and string display #1549

zkry commented Jan 20, 2018

darkowlzz left a comment •

edited

Loading

darkowlzz Jan 21, 2018

zkry commented Jan 21, 2018

zkry commented Jan 22, 2018 •

edited

Loading

darkowlzz left a comment

darkowlzz Jan 23, 2018

zkry commented Jan 23, 2018

darkowlzz commented Jan 23, 2018

zkry commented Jan 23, 2018

zevdg Jan 24, 2018

zkry Jan 25, 2018

zevdg left a comment

zevdg Jan 25, 2018 •

edited

Loading

zevdg Jan 25, 2018

zkry Jan 27, 2018

zevdg Jan 25, 2018

darkowlzz commented Feb 5, 2018

zkry commented Feb 6, 2018

zevdg left a comment

zkry commented Feb 7, 2018

sdboyer left a comment

zkry commented Feb 26, 2018

darkowlzz left a comment

Dep status -f doc and string display #1549

Dep status -f doc and string display #1549

Conversation

zkry commented Jan 20, 2018

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Which issue(s) does this PR fix?

darkowlzz left a comment • edited Loading

Choose a reason for hiding this comment

darkowlzz Jan 21, 2018

Choose a reason for hiding this comment

zkry commented Jan 21, 2018

zkry commented Jan 22, 2018 • edited Loading

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz Jan 23, 2018

Choose a reason for hiding this comment

zkry commented Jan 23, 2018

darkowlzz commented Jan 23, 2018

zkry commented Jan 23, 2018

zevdg Jan 24, 2018

Choose a reason for hiding this comment

zkry Jan 25, 2018

Choose a reason for hiding this comment

zevdg left a comment

Choose a reason for hiding this comment

zevdg Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

zevdg Jan 25, 2018

Choose a reason for hiding this comment

zkry Jan 27, 2018

Choose a reason for hiding this comment

zevdg Jan 25, 2018

Choose a reason for hiding this comment

darkowlzz commented Feb 5, 2018

zkry commented Feb 6, 2018

zevdg left a comment

Choose a reason for hiding this comment

zkry commented Feb 7, 2018

sdboyer left a comment

Choose a reason for hiding this comment

zkry commented Feb 26, 2018

darkowlzz left a comment

Choose a reason for hiding this comment

darkowlzz left a comment •

edited

Loading

zkry commented Jan 22, 2018 •

edited

Loading

zevdg Jan 25, 2018 •

edited

Loading