add new notes output to get command #153
Conversation
|
Nice! Check out my comment in your deps PR re an upcoming change. What do you think about this? Rather than --notes, I wonder if a flag like --output would be better? For example, it leaves the implementation open for future types:
|
|
@chelnak all set I think |
| type outputEnum string | ||
|
|
||
| const ( | ||
| outputStandard outputEnum = "standard" | ||
| outputNotes outputEnum = "notes" | ||
| ) | ||
|
|
||
| func (e *outputEnum) String() string { | ||
| return string(*e) | ||
| } | ||
|
|
||
| func (e *outputEnum) Set(v string) error { | ||
| switch v { | ||
| case string(outputStandard), string(outputNotes): | ||
| *e = outputEnum(v) | ||
| return nil | ||
| default: | ||
| return fmt.Errorf(`must be one of %s or %s`, outputStandard, outputNotes) | ||
| } | ||
| } | ||
|
|
||
| func (e *outputEnum) Type() string { | ||
| return "outputEnum" | ||
| } |
There was a problem hiding this comment.
How about we keep the implementation inside the writer package?
I'll explain what I mean over the next few comments.
There was a problem hiding this comment.
This is only for handling the command line arguments. Open to suggestions on how to remove some of the duplication.
| return "outputEnum" | ||
| } | ||
|
|
||
| var outputTemplate = outputStandard |
There was a problem hiding this comment.
outputTemplate could become outputType which is just a string.
| var outputTemplate = outputStandard | |
| var outputType string |
| switch outputTemplate { | ||
| case outputStandard: | ||
| tmplSrc = writer.TmplSrcStandard | ||
| case outputNotes: | ||
| tmplSrc = writer.TmplSrcNotes | ||
| } | ||
|
|
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| if err := writer.Write(&buf, changelog); err != nil { | ||
| if err := writer.Write(&buf, tmplSrc, changelog); err != nil { |
There was a problem hiding this comment.
writer.Write should now expect a string
| if err := writer.Write(&buf, tmplSrc, changelog); err != nil { | |
| if err := writer.Write(&buf, outputType, changelog); err != nil { |
| var changelog changelog.Changelog | ||
| var err error | ||
|
|
||
| if printLatest { | ||
| changelog, err = get.GetLatest(fileName) | ||
| } else if printVersion != "" { | ||
| changelog, err = get.GetVersion(fileName, printVersion) | ||
| } else if outputTemplate == outputNotes { |
There was a problem hiding this comment.
For this comparison, we can just do a simple string one:
| } else if outputTemplate == outputNotes { | |
| } else if outputType == "notes" { |
There was a problem hiding this comment.
outputType makes sense, but comparing to a string does not. All of this ties back to command flag validation and using constants to expose issues early, at compile time.
| getCmd.Flags().Var( | ||
| &outputTemplate, | ||
| "output", | ||
| fmt.Sprintf(`Output template. allowed: "%s" or "%s"`, outputStandard, outputNotes), | ||
| ) |
There was a problem hiding this comment.
For the flag definition, we can use StringVar and set a default value of standard.
| getCmd.Flags().Var( | |
| &outputTemplate, | |
| "output", | |
| fmt.Sprintf(`Output template. allowed: "%s" or "%s"`, outputStandard, outputNotes), | |
| ) | |
| getCmd.Flags().StringVar( | |
| &outputType, | |
| "output", | |
| "standard", | |
| fmt.Sprintf(`Output template. allowed: "%s" or "%s"`, "standard", "notes"), | |
| ) |
There was a problem hiding this comment.
Where does flag value validation happen?
| @@ -45,7 +45,7 @@ var newCmd = &cobra.Command{ | |||
| return err | |||
| } | |||
|
|
|||
| if err := writer.Write(f, changelog); err != nil { | |||
| if err := writer.Write(f, writer.TmplSrcStandard, changelog); err != nil { | |||
There was a problem hiding this comment.
Simple change here - writer.Write just takes standard.
| if err := writer.Write(f, writer.TmplSrcStandard, changelog); err != nil { | |
| if err := writer.Write(f, "standard", changelog); err != nil { |
There was a problem hiding this comment.
Doesn't this trade compile time for run-time errors and then it needs more tests to prevent invalid values when it's implicit using types and constants?
| const ( | ||
| TmplSrcStandard = tmplStandard | ||
| TmplSrcNotes = tmplNotes | ||
| ) |
There was a problem hiding this comment.
We would no longer need to expose these two consts
| TmplSrcNotes = tmplNotes | ||
| ) | ||
|
|
||
| func Write(writer io.Writer, tmplSrc string, changelog changelog.Changelog) error { |
There was a problem hiding this comment.
And Write would become:
func getTemplate(s string) string {
switch s {
case "standard":
return tmplStandard
case "notes":
return tmplNotes
default:
return tmplStandard
}
}
func Write(writer io.Writer, outputType string, changelog changelog.Changelog) error {
tmplSrc := getTemplate(outputType)
tmpl, err := template.New("changelog").Funcs(template.FuncMap{
"getFirstCommit": func() string {
git := gitclient.NewGitClient(exec.Command)
commit, err := git.GetFirstCommit()
if err != nil {
return ""
}
return commit
},
}).Parse(tmplSrc)
if err != nil {
return err
}
return tmpl.Execute(writer, changelog)
}
| @@ -36,23 +62,33 @@ This command is useful for creating and updating Release notes in GitHub. | |||
| RunE: func(command *cobra.Command, args []string) error { | |||
| fileName := configuration.Config.FileName | |||
|
|
|||
| var tmplSrc string | |||
There was a problem hiding this comment.
tmplSrc is no longer needed.
| var tmplSrc string |
|
Summing up my previous thoughts.. So yeah, I was going down the runtime route with my suggestions. To be honest though, I'm perfectly happy with what you have in the PR. My main concerns are always things like simplicity and whether the app follows the same patterns elsewhere! |
|
From the comments, I don't think anything needs to change. Can this be merged? |
|
Let's do it 🙂 |
Basically an alternate template to the changelog format that only works with a single "Entry" and doesn't output the changelog header or footer.
A "full" changelog style entry is usually not desired when adding to annotated tags or a Github release message.
Ideally, as this extension is GH specific, I would like to see the release notes more release friendly (automatic contributors section).
Instead of this:
Output this for release notes:
Which ends up looking like: