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

Open
slewiskelly opened this issue Sep 17, 2019 · 6 comments
Open

wiki: CodeReviewComments additions #34336

slewiskelly opened this issue Sep 17, 2019 · 6 comments

Comments

@slewiskelly
Copy link

@slewiskelly slewiskelly commented Sep 17, 2019

Propose adding a handful of sections to the CodeReviewComments page. The sections that follow provide advice given during Go readability reviews at Google.

Import _

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.

Avoid blank imports in library packages, even if the library indirectly depends on them. Containing side-effect imports in the main package helps control dependencies, and makes it possible to write tests that rely on a different side-effect without conflict.

If you create a library package that indirectly depends on a side-effect import, leave a comment reminding users to import that package in their main package.

Switch Break

Avoid the use of redundant break statements without target labels at the ends of switch clauses. Unlike in C or Java, switch clauses in Go automatically break, and a fallthrough statement is needed to achieve the C-style behavior.

Use a comment if you want to clarify the purpose of an empty clause.

Prefer:

switch x {
case "A", "B":
  buf.WriteString(x)
case "C":
  // Handled outside of the switch statement
default:
  return fmt.Errorf("unknown value: %q", x)
}

and not:

switch x {
case "A", "B":
  buf.WriteString(x)
  break  // Redundant
case "C":
  break // Redundant
default:
  return fmt.Errorf("unknown value: %q", x)
}

Type Aliases

Use a type definition (type T1 T2) to define a new type.

Use an alias declaration (type T1 = T2) to refer to an existing type without defining a new type.

Prefer type definitions over alias declarations.

Use %q

Go's format functions (fmt.Printf and friends) have a %q verb which prints a string inside quotation marks. It should be used in preference to doing the equivalent manually.

Prefer:

 fmt.Printf("value %q looks like English text", someText)

and not:

 fmt.Printf("value '%s' looks like English text", someText)

Also, note that using %q is a good idea in output intended for humans where the input value could possibly be empty. It can be very hard to notice a silent empty string, but "" stands out clearly as such.

@toothrot toothrot added this to the Unreleased milestone Sep 17, 2019
@toothrot
Copy link
Contributor

@toothrot toothrot commented Sep 17, 2019

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 17, 2019

These all seem fine to me. I started a thread on golang-dev to raise awareness of the suggested changes (https://groups.google.com/forum/?pli=1#!topic/golang-dev/u1RugupaP_Y). Let's give it a few days. Thanks.

@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Sep 17, 2019

%q may produce unnecessary escaping when printing e.g. JSON. Maybe at least mention %#q?

@cespare
Copy link
Contributor

@cespare cespare commented Sep 17, 2019

I don't think I agree with the Type Aliases section.

Use a type definition (type T1 T2) to define a new type.

Use an alias declaration (type T1 = T2) to refer to an existing type without defining a new type.

These two sentences describe how the language works, which I don't think is the purpose of the document.

Prefer type definitions over alias declarations.

It's not clear to me that this is a matter of style. Usually you need the semantics of one or the other.

@vcabbage
Copy link
Member

@vcabbage vcabbage commented Sep 17, 2019

The import _ advice seems situational to me. For example, I would expect the import for a database driver to be in the same package as where the sql.Open happens. That could be in main or, as is typical in my code, it could happen in a package that abstracts much of the DB logic.

I also think importing in main rather than where a package is used is more likely to result in unused imports since removing the package which requires it will not remove the dependency.

The advice seems more applicable to library authors than those writing applications that are comprised of multiple packages.

@ChrisHines
Copy link
Contributor

@ChrisHines ChrisHines commented Sep 18, 2019

%q also obfuscates Windows file paths by doubling the \ path separators.

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
7 participants
You can’t perform that action at this time.