Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SampleGen: LRO #122

Merged
merged 6 commits into from Apr 12, 2019
Merged

SampleGen: LRO #122

merged 6 commits into from Apr 12, 2019

Conversation

yihanzhen
Copy link
Contributor

No description provided.

@yihanzhen
Copy link
Contributor Author

@googleapis/samplegen PTAL

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

It looks like you've checked in a binary. Was that by accident?

p("}")

lroConf := methConf.LongRunning
var typ initType
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Personally, I like to handle a condition (i.e. errors) directly and have happy path be the "fall through". It's easier to read imo.

if lroConf.ReturnType == "" {
  return errors.E(nil, "LRO return type not given")
}

typ := initType{desc: g.descInfo.Type["."+lroConf.ReturnType]}

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

That seems to be the Go prefered style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG

Copy link

@beccasaurus beccasaurus left a comment

Choose a reason for hiding this comment

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

baseline LGTM (ref)

@yihanzhen
Copy link
Contributor Author

Binary removed. PTAL @noahdietz

p("}")

lroConf := methConf.LongRunning
var typ initType
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

That seems to be the Go prefered style

@@ -0,0 +1,58 @@
// Copyright 2018 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

2019

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, the time of the test is locked in 2018 so that new years don't make all tests fail.

op, err := c.LroMethod(ctx, req)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would put a blank line after the error-checking clause (ie after this line and after what is currently line 45).

Totally fine with discussing/implementing such cosmetic changes in a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Same thing on line 45


req := &foopb.LroInType{
}
op, err := c.LroMethod(ctx, req)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it clearer to call this op or lro? I would lean toward the latter, but happy to conform to any conventions we may have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the google-cloud-go repository, we use op in our method snippets. So maybe we stick with op here. lro would also be a good name though.

@vchudnov-g
Copy link
Contributor

(arg, I thought I submitted my review yesterday, but as I often do, I forgot to hit "Submit") ☹️

@@ -0,0 +1,58 @@
// Copyright 2018 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, the time of the test is locked in 2018 so that new years don't make all tests fail.

@yihanzhen yihanzhen merged commit d01c9e9 into googleapis:master Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants