Output 'Code generated by' comment if arg is set #94

jennybuckley · 2018-01-11T18:33:47Z

The format of the comment is:

// Code generated by <gemerator-name>. DO NOT EDIT.

It will be located directly above the package declaration.

k8s-reviewable · 2018-01-11T18:33:51Z

This change is

sttts · 2018-01-11T20:03:20Z

Do we need a way to customize the string? If the output language is not Go (do we have that? openapi or protobuf?), the string must be customizable (not via flag, but via code, i.e. optionally).

lavalamp · 2018-01-11T20:48:25Z

Yes, one of these generators makes protobuf...

…

On Thu, Jan 11, 2018 at 12:03 PM, Dr. Stefan Schimanski < ***@***.***> wrote: Do we need a way to customize the string? If the output language is not Go (do we have that? openapi or protobuf?), the string must be customizable (not via flag, but via code, i.e. optionally). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#94 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnglv1lZp4rhnVT9AlP4CjDWDyZPeWyks5tJmkIgaJpZM4RbQxS> .

jennybuckley · 2018-01-11T22:06:43Z

I believe the proto files already have https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/admission/v1beta1/generated.proto#L18

// This file was autogenerated by go-to-protobuf. Do not edit it manually!

which this pr shouldn't affect.

thockin · 2018-01-12T04:55:54Z

args/args.go

@@ -92,6 +96,7 @@ func (g *GeneratorArgs) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVarP(&g.OutputPackagePath, "output-package", "p", g.OutputPackagePath, "Base package path.")
 	fs.StringVarP(&g.OutputFileBaseName, "output-file-base", "O", g.OutputFileBaseName, "Base name (without .go suffix) for output files.")
 	fs.StringVarP(&g.GoHeaderFilePath, "go-header-file", "h", g.GoHeaderFilePath, "File containing boilerplate header text. The string YEAR will be replaced with the current 4-digit year.")
+	fs.BoolVar(&g.GeneratedByComment, "go-generated-by-comment", g.GeneratedByComment, `Add a "Code generated by" comment into each file.`)


do we even need a flag?

I would drop it. It's something the generator builder decides.

That seems right to me

sttts · 2018-01-12T08:32:33Z

which this pr shouldn't affect.

But it should be replaced with the generic mechanism, right? But if protobuf is happy with // we are fine. And if another generator produces code in a language that doesn't like //, the comment can be disabled and created manually.

jennybuckley · 2018-01-12T18:40:00Z

@sttts it should now address the comments.

Replacing GENERATOR_NAME with the name of the generator seems to be in line with how YEAR is handled, and this should let generator builders decide if they want to use // or something else, or decide if they want to skip this entirely and define it in their own code.

I wonder if "" might be a more sensible default than "// Code generated by GENERATOR_NAME. DO NOT EDIT." so the behavior won't change for any existing generators.

jennybuckley · 2018-01-12T18:57:11Z

Oh looks like #24 is dropping the YEAR functionality, but i guess the pattern of replacing a word that looks like an environment variable with what it means is probably still valid.

sttts · 2018-01-15T09:41:26Z

args/args.go

+		generatorPath := strings.Split(os.Args[0], "/")
+		generatorName := generatorPath[len(generatorPath)-1]
+		generatedByComment = strings.Replace(g.GeneratedByCommentTemplate, "GENERATOR_NAME", generatorName)
+		s := fmt.Sprintf("\n%s\n\n", generatedByComment)


The first newline is not necessary if the file is empty (which it is e.g. in OpenShift)

I think it could be removed entirely, since the licences usually end with an extra \n

Also it would be hard to tell if the the newline should be printed if there is // +build !ignore_autogenerated above the boilerplate

jennybuckley · 2018-01-16T18:56:19Z

args/args.go

+	if g.GeneratedByCommentTemplate != "" {
+		generatorPath := strings.Split(os.Args[0], "/")
+		generatorName := generatorPath[len(generatorPath)-1]
+		generatedByComment = strings.Replace(g.GeneratedByCommentTemplate, "GENERATOR_NAME", generatorName)


should be := and needs an extra argument of -1

jennybuckley · 2018-01-17T18:38:36Z

Looks like I still have to change the example generators to get their outputs to match the test outputs

jennybuckley · 2018-01-17T21:31:41Z

boilerplate/boilerplate.go.txt

@@ -1,5 +1,5 @@
 /*
-Copyright YEAR The Kubernetes Authors.
+Copyright 2017 The Kubernetes Authors.


Pretty sure this is not the right place for this change but since #91 hasn't merged yet, this is necessary to check that the behavior of the example generators isn't affected.

thockin

LGTM except boilerplate delta

thockin · 2018-01-19T05:53:39Z

examples/set-gen/main.go

@@ -35,6 +35,10 @@ import (

 func main() {
 	arguments := args.Default()
+
+	// Override defaults.


@jennybuckley was this only for testing?

Yes, this was to make the tests pass.

should we fix the tests?

imo, the tests are good how they are because they test if the behavior of the example generators have changed based on the changes to the generic gengo code. I could have run the generators and committed the changes to the generated code, instead of modifying the example generators, but that wouldn't show that the old behavior of the example generators can be implemented by setting the new generator argument, and the output will be exactly how it was before.

Yes, please remove the overrides and update the test output in another commit.

jennybuckley · 2018-01-19T19:00:36Z

@thockin removed the boilerplate change and rebased on #91

thockin · 2018-01-19T20:21:22Z

#91 merged

sttts · 2018-01-22T09:41:14Z

args/args.go

@@ -64,6 +65,11 @@ type GeneratorArgs struct {
 	// Where to get copyright header text.
 	GoHeaderFilePath string

+	// If GeneratedByCommentTemplate is set, generate a "Code generated by" comment
+	// above the package declaration of the format defined by this string.


below the boilerplate

sttts · 2018-01-22T09:41:52Z

args/args.go

+
+	if g.GeneratedByCommentTemplate != "" {
+		generatorPath := strings.Split(os.Args[0], "/")
+		generatorName := generatorPath[len(generatorPath)-1]


https://golang.org/pkg/path/#Base

sttts · 2018-01-22T09:42:29Z

examples/deepcopy-gen/main.go

@@ -69,6 +69,7 @@ func main() {

 	// Override defaults.
 	arguments.OutputFileBaseName = "deepcopy_generated"
+	arguments.GeneratedByCommentTemplate = "\n// This file was autogenerated by deepcopy-gen. Do not edit it manually!"


Why is this set explicitly. We should use the default.

sttts · 2018-01-22T09:42:42Z

examples/defaulter-gen/main.go

@@ -54,6 +54,7 @@ func main() {

 	// Override defaults.
 	arguments.OutputFileBaseName = "defaulter_generated"
+	arguments.GeneratedByCommentTemplate = "\n// This file was autogenerated by defaulter-gen. Do not edit it manually!"


we should use the default.

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. code-gen: output golint compliant 'Generated by' comment New PR instead of reopening #58115 because /reopen did not work. This won't be ready to merge until the upstream kubernetes/gengo#94 merges. Once that merges, the second commit will be changed to godep-save.sh and update-staging-godeps.sh, and the last commit will be changed to update-all.sh The failing test is due to the upstream changes not being merged yet ```devel-release-note Go code generated by the code generators will now have a comment which allows them to be easily identified by golint ``` Fixes #56489 Kubernetes-commit: 1eb1c00c44f8f597b9b23a05cd0a8da205c87f8a

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2018

thockin reviewed Jan 12, 2018

View reviewed changes

jennybuckley changed the title ~~Output 'Code generated by' comment if flag is set~~ Output 'Code generated by' comment if arg is set Jan 12, 2018

sttts reviewed Jan 15, 2018

View reviewed changes

jennybuckley commented Jan 16, 2018

View reviewed changes

jennybuckley force-pushed the generated-by branch from c76fa61 to 42c1755 Compare January 16, 2018 19:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 17, 2018

jennybuckley commented Jan 17, 2018

View reviewed changes

thockin reviewed Jan 19, 2018

View reviewed changes

jennybuckley force-pushed the generated-by branch 2 times, most recently from 711080e to 9a20df7 Compare January 19, 2018 18:59

jennybuckley force-pushed the generated-by branch from 9a20df7 to debc696 Compare January 19, 2018 22:06

sttts reviewed Jan 22, 2018

View reviewed changes

jennybuckley force-pushed the generated-by branch 2 times, most recently from dd8bd57 to 3a11997 Compare January 23, 2018 23:11

trshafer mentioned this pull request Mar 27, 2019

Update gengo knative/serving#3544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output 'Code generated by' comment if arg is set #94

Output 'Code generated by' comment if arg is set #94

jennybuckley commented Jan 11, 2018

k8s-reviewable commented Jan 11, 2018

sttts commented Jan 11, 2018

lavalamp commented Jan 11, 2018 via email

jennybuckley commented Jan 11, 2018 •

edited

Loading

thockin Jan 12, 2018

sttts Jan 12, 2018

jennybuckley Jan 12, 2018

sttts commented Jan 12, 2018

jennybuckley commented Jan 12, 2018 •

edited

Loading

jennybuckley commented Jan 12, 2018 •

edited

Loading

sttts Jan 15, 2018

jennybuckley Jan 16, 2018

jennybuckley Jan 16, 2018 •

edited

Loading

jennybuckley Jan 16, 2018

jennybuckley commented Jan 17, 2018

jennybuckley Jan 17, 2018

thockin left a comment

thockin Jan 19, 2018

sttts Jan 19, 2018

jennybuckley Jan 19, 2018

thockin Jan 19, 2018

jennybuckley Jan 19, 2018

sttts Jan 22, 2018

jennybuckley commented Jan 19, 2018

thockin commented Jan 19, 2018

sttts Jan 22, 2018

sttts Jan 22, 2018

sttts Jan 22, 2018

sttts Jan 22, 2018

Output 'Code generated by' comment if arg is set #94

Output 'Code generated by' comment if arg is set #94

Conversation

jennybuckley commented Jan 11, 2018

k8s-reviewable commented Jan 11, 2018

sttts commented Jan 11, 2018

lavalamp commented Jan 11, 2018 via email

jennybuckley commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jan 12, 2018

jennybuckley commented Jan 12, 2018 • edited Loading

jennybuckley commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley commented Jan 17, 2018

Choose a reason for hiding this comment

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley commented Jan 19, 2018

thockin commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley commented Jan 11, 2018 •

edited

Loading

jennybuckley commented Jan 12, 2018 •

edited

Loading

jennybuckley commented Jan 12, 2018 •

edited

Loading

jennybuckley Jan 16, 2018 •

edited

Loading