Allow go2idl to generate non-Go file types #17853
Conversation
|
Labelling this PR as size/L |
k8s-github-robot
added
the
size/L
|
GCE e2e test build/test passed for commit cf08e17f63e3de23268053567f47488c1efc03fe. |
|
@smarterclayton - it seems that something doesn't compile correctly: 16:22:23 # k8s.io/kubernetes/cmd/libs/go2idl/client-gen/generators Can you please fix it? |
| } | ||
|
|
||
| // format should be one line only, and not end with \n. | ||
| func addIndentHeaderComment(b *bytes.Buffer, format string, args ...interface{}) { | ||
| if b.Len() > 0 { | ||
| fmt.Fprintf(b, "\n\t// "+format+"\n", args...) | ||
| fmt.Fprintf(b, "\n// "+format+"\n", args...) |
There was a problem hiding this comment.
nit: I'm not sure we need it - it will be added by gofmt anyway (note that the whole code is not formatted, to formatting just these comments makes it non-consistent).
There was a problem hiding this comment.
Yeah, and it makes formatting proto idl (which has no gofmt equivalent)
harder.
On Nov 27, 2015, at 5:20 AM, Wojciech Tyczynski notifications@github.com
wrote:
In cmd/libs/go2idl/generator/execute.go
#17853 (comment):
}
// format should be one line only, and not end with \n.
func addIndentHeaderComment(b *bytes.Buffer, format string, args ...interface{}) {
if b.Len() > 0 {
fmt.Fprintf(b, "\n\t// "+format+"\n", args...)fmt.Fprintf(b, "\n// "+format+"\n", args...)
nit: I'm not sure we need it - it will be added by gofmt anyway (note that
the whole code is not formatted, to formatting just these comments makes it
non-consistent).
—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/17853/files#r46033739.
There was a problem hiding this comment.
That's a good point.
I'm completely fine with adding it, although, I'm not sure if that helps much for protos (but probably it's better with that).
|
LGTM - (just two very minor comments). Feel-free to self-lgtm after fixing compile errors |
|
Ah, new code. Will fix. |
|
@smarterclayton - thanks LGTM |
smarterclayton
force-pushed
the
non_go_idl
branch
from
cf08e17 to
c0ea455
Compare
smarterclayton
added
the
lgtm
|
Errors fixed, applying LGTM |
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
@k8s-bot unit test this |
|
GCE e2e test build/test passed for commit c0ea45520cc1b1a0f2b7e19ec3aa872e9ee7594b. |
Remove some indentation from file generation for specific languages, allow SnippetWriter's io.Writer to be accessed, and add Path to types.Name for languages where Package and Path can be disjoint.
smarterclayton
force-pushed
the
non_go_idl
branch
from
c0ea455 to
72df8bb
Compare
|
PR changed after LGTM, removing LGTM. |
k8s-github-robot
removed
the
lgtm
|
GCE e2e build/test failed for commit 72df8bb. |
|
ok to test |
|
GCE e2e test build/test passed for commit 72df8bb. |
smarterclayton
added
the
lgtm
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 72df8bb. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 72df8bb. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Remove some indentation from file generation for specific languages,
allow SnippetWriter's io.Writer to be accessed, and add Path to
types.Name for languages where Package and Path can be disjoint.
Extracted from #16649, prereq for protobuf generation support.