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

wiki: CodeReviewComments change #31081

Open
kurin opened this issue Mar 27, 2019 · 5 comments
Open

wiki: CodeReviewComments change #31081

kurin opened this issue Mar 27, 2019 · 5 comments
Labels
Milestone

Comments

@kurin
Copy link

@kurin kurin commented Mar 27, 2019

The code review comments state: "Avoid renaming imports except to avoid a name collision; good package names should not require renaming." However, a pattern I am used to is to rename a protobuf import "pb" or, if there are many protobuf imports, "somethingpb" where "something" is usually pretty short.

Should there be a specific acknowledgement of this here?

@agnivade
Copy link
Contributor

@agnivade agnivade commented Mar 27, 2019

@dgryski
Copy link
Contributor

@dgryski dgryski commented Mar 30, 2019

I agree it's a convention. The question is does every convention need to be listed in this wiki page.

@andybons
Copy link
Member

@andybons andybons commented Mar 31, 2019

I don’t think this convention needs to be specifically called out, but perhaps @neild has an opinion.

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 12, 2019

The justification for the comment already says “good package names should not require renaming”, so it already mostly does not apply to package names that are not “good” (such as those imposed by dependencies translated from non-native-Go sources).

@bcmills bcmills added this to the Unreleased milestone Apr 12, 2019
@andybons
Copy link
Member

@andybons andybons commented Apr 17, 2019

Per discussion with @golang/proposal-review, seems fine to add a note about protobufs.

What's the proposed wording?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.