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

Jump to bottom

wiki: CodeReviewComments change #29229

Open
matloob opened this issue Dec 13, 2018 · 5 comments
Open

wiki: CodeReviewComments change #29229

matloob opened this issue Dec 13, 2018 · 5 comments
Labels
Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unreleased

Comments

@matloob
Copy link
Contributor

matloob commented Dec 13, 2018

Is it okay to add an ImportsBlank section to the CodeReviewComments with the following text?

ImportBlank

Packages that are imported only for their side effects (using the syntax import _ "pkg") should only be imported in the main package of a program, or in tests
that require them.
@matloob
Copy link
Contributor Author

matloob commented Dec 13, 2018

Lint already gives an error for this

@ianlancetaylor
Copy link
Contributor

CC @andybons

@andybons
Copy link
Member

I'm not opposed to it, but I'd like there to be more justification alongside the recommendation so that people understand why it's there.

@bcmills
Copy link
Contributor

bcmills commented Dec 19, 2018

I think the actual advice should probably be a bit more nuanced. Perhaps something like:

Packages that are imported only for their side effects (using import _) may add overhead at compile time (to link in unneeded packages) and at run time (to initialize those packages). Put such imports only in packages that cannot function at all without them, such as in main packages and in tests that require the side effect of the import.

@bcmills bcmills added Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Dec 19, 2018
@bcmills bcmills added this to the Unreleased milestone Dec 19, 2018
@matloob
Copy link
Contributor Author

matloob commented Jan 10, 2019

@andybons what do you think of bcmills' wording?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@andybons @ianlancetaylor @bcmills @matloob