Skip to content

initial work on samplegen#3

Merged
pongad merged 2 commits intogoogleapis:masterfrom
pongad:samplegen
Sep 20, 2018
Merged

initial work on samplegen#3
pongad merged 2 commits intogoogleapis:masterfrom
pongad:samplegen

Conversation

@pongad
Copy link
Copy Markdown
Contributor

@pongad pongad commented Sep 18, 2018

No description provided.

vchudnov-g
vchudnov-g reviewed Sep 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@vchudnov-g vchudnov-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as scaffolding, but there are some changes I'd make before committing (imports in the generated sample, etc) so that the generated code runs, and I'd add some strategic TODOs throughout.

Aside from that, minor comments about initialisms and descriptive names.

Comment thread cmd/gen-go-sample/gapic.go Outdated
}
}

type GapicValueSet struct {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/gapic.go Outdated
Methods []GapicMethod
}

type GapicMethod struct {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/gapic.go Outdated
}
}

type GapicValueSet struct {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/main.go Outdated

type generator struct {
desc descriptor.FileDescriptorSet
dinfo pbinfo.Info

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/main.go Outdated
gapic GapicConfig

pt printer.P
imps map[pbinfo.ImportSpec]bool

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/main.go Outdated

p := g.pt.Printf

p("// [START %s]", valSet.ID)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/main.go Outdated
var file bytes.Buffer
file.WriteString("package main\n")
file.WriteString("import(\n")
for _, imp := range imps {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/main_test.go
Comment thread cmd/gen-go-sample/testdata/sample.want Outdated
@@ -0,0 +1,22 @@
// [START my_value_set]

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment thread cmd/gen-go-sample/testdata/sample.want
// [START my_value_set]

func sampleMyMethod() {
ctx := context.Background()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

yihanzhen
yihanzhen approved these changes Sep 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@yihanzhen yihanzhen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much easier to understand than what I expected... One question though. Other than that LGTM

Comment thread cmd/gen-go-sample/test.bash
fi

./gen-go-sample \
-gapic "$GOOGLEAPIS/google/cloud/language/v1/language_gapic.yaml" \

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@pongad pongad requested a review from beccasaurus September 20, 2018 22:36
@pongad pongad merged commit fa9b114 into googleapis:master Sep 20, 2018
@pongad pongad deleted the samplegen branch September 20, 2018 23:36
@pongad
Copy link
Copy Markdown
Contributor Author

pongad commented Sep 21, 2018

@hzyi-google I'm glad to hear! :P

julieqiu referenced this pull request in julieqiu/gapic-generator-go Oct 5, 2023
@julieqiu
go.mod: upgrade golang.org/x/net
10ba345 
Vulnerability #1: GO-2023-1571
    Denial of service via crafted HTTP/2 stream in net/http and golang.org/x/net
  More info: https://pkg.go.dev/vuln/GO-2023-1571
  Module: golang.org/x/net
    Found in: golang.org/x/net@v0.0.0-20210405180319-a5a99cb37ef4
    Fixed in: golang.org/x/net@v0.7.0
    Example traces found:
      #1: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.ErrCode.String
      #2: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.FrameType.String
      #3: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.Setting.String
      #4: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.SettingID.String
      googleapis#5: internal/printer/printer.go:62:13: printer.P.Printf calls fmt.Fprintf, which eventually calls http2.chunkWriter.Write
      googleapis#6: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.duplicatePseudoHeaderError.Error
      googleapis#7: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.headerFieldNameError.Error
      googleapis#8: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.headerFieldValueError.Error
      googleapis#9: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.pseudoHeaderError.Error
      googleapis#10: internal/gengapic/options.go:172:21: gengapic.transport.String calls fmt.Sprintf, which eventually calls http2.writeData.String
@quartzmo quartzmo mentioned this pull request Mar 25, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vchudnov-g vchudnov-g vchudnov-g left review comments

@beccasaurus beccasaurus Awaiting requested review from beccasaurus

+1 more reviewer

@yihanzhen yihanzhen yihanzhen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pongad @vchudnov-g @yihanzhen