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

Cleanup of grpc go generation code #136

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

markmandel
Copy link
Member

I'd previously generated this in two places, and looking at it through Godoc, it seemed like a code smell.

Having the generated code in a single place seems like the cleaner option.

I'd previously generated this in two places, and looking
at it through Godoc, it seemed like a code smell.

Having the generated code in a single place seems like the cleaner
option.
@markmandel markmandel added the kind/cleanup Refactoring code, fixing up documentation, etc label Mar 9, 2018
@markmandel markmandel added this to the 0.2 milestone Mar 9, 2018
@markmandel
Copy link
Member Author

@enocom Curious if you see any issues here?

@enocom
Copy link
Contributor

enocom commented Mar 9, 2018

@markmandel Yeah, definitely don't need two generated files. The C++ generated stuff might even be deleted too.

In theory, the proto file is enough, but since we're providing a Go library, we might as well have the generated code for people consuming things within pkg.

@markmandel markmandel requested a review from enocom March 9, 2018 23:27
@markmandel
Copy link
Member Author

Personally, I prefer to keep generated files in the repo - makes clear what is going on (the CRD client is all generated too) - as long as it's header'd appropriately.

Kinda like vendoring dependencies 😄

@markmandel
Copy link
Member Author

Also makes it go-gettable

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: a2980d51-f540-4758-95e6-64c07f487614

The following development artifacts have been built, and will exist for the next 30 days:

@enocom
Copy link
Contributor

enocom commented Mar 9, 2018

SGTM. Duplicate unnecessary, for sure.

Copy link
Contributor

@enocom enocom left a comment

Choose a reason for hiding this comment

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

LGTM.

@enocom enocom merged commit b47d77f into googleforgames:master Mar 9, 2018
@markmandel markmandel deleted the cleanup/go-sdk branch March 9, 2018 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Refactoring code, fixing up documentation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants