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

proposal: cmd/gofmt: minimize adjacent whitespace changes on struct layout edits #37399

Closed
ohir opened this issue Feb 24, 2020 · 5 comments
Closed
Labels
Projects
Milestone

Comments

@ohir
Copy link

@ohir ohir commented Feb 24, 2020

Proposal: Minimize adjacent whitespace changes on struct layout edits.

Author: Ohir Ripe [Wojciech S. Czarnecki]

Last updated: 2020/02/24

Discussion at https://golang.org/issues/37399

Supersedes: #37299

Abstract

I propose to change the gofmt rule regarding formatting of struct declarations to use fixed width fields both for identifier and type then let lines that does not fit this fixed schema to stay outlier.

Proposed formatting trades off readability of code that uses long identifiers (not common in Go) for improved (smaller) size of changesets introduced by adding, removing, or renaming a field in a struct declaration.

New formatting would be turned on by go init by a gofmt version directive added to the go.mod file.

Background

With current gofmt rules slight changes to a struct declaration make for avalanche changes across declaration whitespace due to gofmt trying to layout struct declaration "pretty". Produced changeset size make diffs unreadable if any field identifier or type changes its length.

Proposal

New formatting is in force if gofmt current version is equal to or higer than provided in the go.mod file. New format fixes type and comment fields to be of fixed width, with start positions relative to current indent (cindent) position:

  1. R1 fix type start position at cindent + 12
  2. R2 fix comment start position at cindent + 24
  3. R3 lines that do not fit in above fixed schema stay outlier,
    ie. adjusted are only parts that can fit.
//₀₀₀₀₀₀A₁₁₁₁₁₁₁B₁₁₂₂₂₂₂C₂₂₂₂₃₃₃D₃₃₃₃₃₃₄E₄₄₄₄₄₄₄F₅₅₅₅₅₅₅G Tabstop
//³⁴⁵⁶⁷⁸⁹⁰¹²³⁴⁵⁶⁷⁸⁹⁰¹²³⁴⁵⁶⁷⁸⁹⁰¹²³⁴⁵⁶⁷⁸⁹⁰¹²³⁴⁵⁶⁷⁸⁹⁰¹²³⁴⁵⁶⁷ Columns
//
//      Ident       Type        Comment
type NameOf struct {
        identA      int         // midlen 
        id          int64       // short
        longlongname string     // long name outlier
        other       longlongtype // long type outlier
        longlongname longlongtype // both fields do not fit
}

Rationale

As explained in the background section.
This change will benefit all Go programmers that use source control.

Compatibility

This feature will work for a code where gofmt 1.xx directive is present
in the go.mod file. Ie. after go init or author decided to use new formatting rules.

Implementation

This proposal is trivial regarding the formatting part, but otherwise it is not, as it proposes to make gofmt be aware of go.mod contents.


@ianlancetaylor ianlancetaylor changed the title cmd/gofmt: proposal: Minimize adjacent whitespace changes on struct layout edits proposal: cmd/gofmt: minimize adjacent whitespace changes on struct layout edits Feb 24, 2020
@gopherbot gopherbot added this to the Proposal milestone Feb 24, 2020
@gopherbot gopherbot added the Proposal label Feb 24, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 24, 2020

Having choices of formatting based on directives in go.mod does not strike me as a good idea. gofmt is invoked in many different environments by different editors, revision control import hooks, etc. To require all those environments to preserve the go.mod directive is a big ask. And the benefit seems small.

If we're going to change the formatting, let's change the formatting.

(And then let's decide that gofmt is imperfect but is good enough, and let's not change the formatting. But that's just an opinion, not an argument.)

@ohir
Copy link
Author

@ohir ohir commented Feb 24, 2020

Having choices of formatting based on directives in go.mod does not strike me as a good idea.

This go.mod ‘flag’ is not meant to give a choice — it protects repositories of existing code against massive changes induced by gofmt run from CI scripts.

To require all those environments to preserve the go.mod directive is a big ask.

All these environments must now preserve go 1.xx directive at top. The gofmt.1.xx is no differ in this regards. In fact just about any tool does not touch things it does not understand — at least it is supposed to behave such.

Could gofmt just use go 1.xx as an old/new format switcher?

And the benefit seems small.

I beg to differ. It is not unusual to have 20+ lines changesets due to a typo corrected in an identifier.

If we're going to change the formatting, let's change the formatting.

Any change in default formatting needs to account for billion lines of already written code. Ie. it must recognize "old code" and "new code", in one or another way.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 24, 2020

It's not obvious to me that editors and revision control per-submit hooks need to know the go 1.xx directive. Why would they?

The implication that this change would cause massive changes sounds like a good reason to not do it. Why is the benefit worth the cost?

@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

The design of gofmt from the start is that there are no options: go code looks consistent no matter what. As @robpike's proverb goes, "Gofmt's style is no one's favorite, yet gofmt is everyone's favorite."

It's very important to the design that there aren't config knobs. This is a non-starter. Closing.

@rsc rsc closed this Feb 26, 2020
@rsc rsc added this to Declined in Proposals Feb 26, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Feb 26, 2020

Also note that most version control supports whitespace-insensitive diffs, like git diff -b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
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.