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 #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

Comments

@matloob
Copy link
Contributor

@matloob 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 matloob commented Dec 13, 2018

Lint already gives an error for this

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 13, 2018

@andybons
Copy link
Member

@andybons andybons commented Dec 13, 2018

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
Member

@bcmills 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.

@matloob
Copy link
Contributor Author

@matloob 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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