added template for output #83
Conversation
There was a problem hiding this comment.
Thanks! Added some comments.
One idea for improving this would be to instead provide a function that parses the template, using the default value if the supplied template is empty. This would avoid having to parse the template multiple times, and the calls to Display() would be replaced by calls to t.Execute(), with a single call to the parsing function up front to get the template to use.
backvendor/display.go
Outdated
| //Display a Reference using a template | ||
| func Display(writer io.Writer, customTemplate string, ref *Reference) error { | ||
| if customTemplate == "" { | ||
| tmpl, err := template.New("output").Parse(defaultTemplate) |
There was a problem hiding this comment.
In this case perhaps we could just customTemplate = defaultTemplate (the caller is not affected by this) and fall through to the case where a custom template has been supplied.
backvendor/display.go
Outdated
| } | ||
| err = tmpl.Execute(writer, ref) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Once tmpl.Execute(writer, ref) is the last thing to do, If we aren't going to use anything like errors.Wrap() on the error we could just immediately return that result and leave the checking to the caller, e.g. return tmpl.Execute(writer, ref).
| @@ -36,6 +36,7 @@ var importPath = flag.String("importpath", "", "top-level import path") | |||
| var depsFlag = flag.Bool("deps", true, "show vendored dependencies") | |||
| var excludeFrom = flag.String("exclude-from", "", "ignore directory entries matching globs in `exclusions`") | |||
| var debugFlag = flag.Bool("debug", false, "show debugging output") | |||
| var template = flag.String("template", "", "go template to use for output with Rev, Tag and Ver") | |||
There was a problem hiding this comment.
I wonder if it's worth putting the default value here instead of having to work out whether to use a fallback in Display()? (But don't want to clutter the help output, so not sure...)
main.go
Outdated
| func display(template string, name string, ref *backvendor.Reference) { | ||
| var builder strings.Builder | ||
| builder.WriteString(name) | ||
| backvendor.Display(&builder, template, ref) |
There was a problem hiding this comment.
Probably want to check the error.
backvendor/display.go
Outdated
|
|
||
| func (t TemplateError) Error(template template.Template) string { | ||
| return fmt.Sprintf("display: Error creating template from %v", template) | ||
| } |
backvendor/display_test.go
Outdated
| ) | ||
|
|
||
| func TestDisplayDefault(t *testing.T) { | ||
| const expected string = "@4309345093405934509 =v0.0.1 ~v0.0.0.20181785" |
There was a problem hiding this comment.
You can remove string from this declaration (and the other similar ones) as the type is inferred from the value.
| const expected string = "@4309345093405934509 =v0.0.1 ~v0.0.0.20181785" | ||
| ref := &Reference{Rev: "4309345093405934509", Tag: "v0.0.1", Ver: "v0.0.0.20181785"} | ||
| var builder strings.Builder | ||
| var tmpl, err = Display("") |
There was a problem hiding this comment.
This would normally be written tmpl, err := Display("")
backvendor/display_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| tmpl.Execute(&builder, ref) |
There was a problem hiding this comment.
We should probably check for errors from this call, and the other similar ones.
backvendor/display_test.go
Outdated
|
|
||
| func TestDisplayNoTag(t *testing.T) { | ||
| const expected string = "@v0.0.0.20181785 ~4309345093405934509" | ||
| ref := &Reference{Rev: "v0.0.0.20181785", Ver: "4309345093405934509"} |
There was a problem hiding this comment.
The Rev and Ver fields look like their values are swapped. Rev is a revision (commit ID), Ver is a semantic version or pseudo-version.
main.go
Outdated
| builder.WriteString(name) | ||
| var tmpl, err = backvendor.Display(template) | ||
| if err != nil { | ||
| log.Warningf("Error parsing supplied template, using default. %s ", err) |
There was a problem hiding this comment.
I'd rather just abort at this point (log.Fatalf) rather than falling back to the default template.
|
The merge history of this branch is quite messy: Could you please re-base it onto master so there is just a single commit with the changes? (I couldn't see how to do this myself without creating a new commit from "git diff master output-template"...) In general it is quite usual with GitHub to have a feature branch whose existing commits you amend, and force-push the changes. Instead of merging the master branch into the feature branch, re-base the feature branch on top of the master branch. GitHub knows how to track all of that -- it just follows the branch name wherever it goes. |
Fixes Issue #33