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

cmd/gofmt: proposal: Minimize adjacent whitespace changes on struct layout edits (user controlled positions). #37299

Closed
ohir opened this issue Feb 19, 2020 · 3 comments
Labels
Projects
Milestone

Comments

@ohir
Copy link

@ohir ohir commented Feb 19, 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/37299

Abstract

I propose to add an opt-in go fmt solution meant to minimize whitespace changes to the block where a code author hinted at desired comments position.

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

  1. Fix relative indent position of the type of the field, then
  2. Let code author hint gofmt where comments should line up.

Comment-start hint has a form of a /< digraph (a cs-mark hereafter) put into the comment that follows the opening brace of the block.

type Some struct { //           /< cs-mark here 
        counter         int     // midlen 
        nce             int64   // short
        longlongname    string  // long
}

Note: cs-mark usually will be used on a struct declaration, but described feature will work for any kind of block.

Proposed new gofmt rules:

New behavior is turned on by a "/<" cs-mark presence
in a first comment of the block.

The tsPos and csPos below stand for type-start and comment-start
positions, respectively. cmPos stands for cs-mark position.

Tabwidth is fixed at 8. Measurements and adjusts are relative to the
block content start position, ie. are indent-independent.

NONE heuristics are used, gofmt just obey the recipe as set here.

Recipe:

R1. Set tsPos to the tabstop that follows the opening brace position.
R2. if cmPos <= tsPos {; cmPos += 8 }     // make sure cmPos > tsPos 
R3. If cmPos is not at tabstop now, advance it to the next tabstop.
R4. Set cs-mark position from the current (adjusted) cmPos.
R5. Rewrite cs-mark line.
R6. Adjust lines in block, shifting relevant parts using spaces.
R7. If text overflows one or the other "start" position, use single space.
    Human can react to spurious overflows moving comment-mark a space or two.
R8. Estabilished tsPos and csPos are valid to the end of block.
    Open question: should we recognize cs-mark in the inner scopes?


On Examples:

// Youri uses tw=8 and writes:
type Other struct { //      /< author set marker here
        counter int // midlen 
        nce int64 // short
        longlongname string // long
}

// e Other struct { //      /< author set comment-start position
//      |---------#-----|         R1 type starts at next ts
//      |-------------------|        given comment-start position
//      |-------------------#---| R3 adjusted comment-start position
//......A.......B.......C.......D.......E.......F.......G.......H...
// 
// Now Youri sees:
type Other struct { //          /< adjusted cs-mark
        counter         int     // midlen 
        nce             int64   // short
        longlongname    string  // long
}

// Bartek tw=4 sees:
type Other struct { //          /< adjusted cs-mark
    counter         int     // midlen 
    nce             int64   // short
    longlongname    string  // long
}

// Beata tw=2 sees:
type Other struct { //          /< adjusted cs-mark
  counter         int     // midlen 
  nce             int64   // short
  longlongname    string  // long
}


// Now Beata writes, she is using tw=2
type Other struct { // /< just near longlongname, as perceived
  counter int // midlen 
  nce int64 // short
  longlongname string // long
}
// e Other struct { // /< author set comment-start position
//      |---------#-----|         R1 type starts at next ts
//      |--------------|             given comment-start position
//      |----------------------|  R2 +8 shift 
//      |--------------#--------| R3 adjusted comment-start position
//......A.......B.......C.......D.......E.......F.......G.......H...
//
// Beata ts=2 now sees:
type Other struct { //          /< adjusted
  counter         int     // midlen 
  nce             int64   // short
  longlongname    string  // long
}
// Bartek ts=4 sees:
type Other struct { //          /< adjusted
    counter         int     // midlen 
    nce             int64   // short
    longlongname    string  // long
}
// Youri ts=8 sees:
type Other struct { //          /< adjusted
        counter         int     // midlen 
        nce             int64   // short
        longlongname    string  // long
}

//
// Excercise R7 rule.
//
// Beata ts=2 writes:
type sn struct { //   /< Beata manually adjusted to longlongname
  field int // midlen 
  n   int64 // short
  longlongname string // r
}
// e sn struct { //   /< Beata manually adjusted to longlongname
//      |------#|         R1 type starts at next ts
//      |-------------|      given comment-start position
//      |-------------#-| R3 adjusted comment-start position
//......A.......B.......C.......D.......E.......F.......G.......H...
//
// Beata ts=2 now sees:
type sn struct { //     /< adjusted 
  field   int     // midlen 
  n       int64   // short
  longlongname string // R7
}

// Bartek ts=4 sees:
type sn struct { //     /< adjusted 
    field   int     // midlen 
    n       int64   // short
    longlongname string // R7
}

// Youri ts=8 sees:
type sn struct { //     /< adjusted 
        field   int     // midlen 
        n       int64   // short
        longlongname string // R7
}

// Beata moved mark by three spaces right:
// 
// Beata ts=2 writes:
type sn struct { //      /< three spaces more
  field int // midlen 
  n   int64 // short
  longlongname string // r
}
// e sn struct { //      /< three spaces more 
//      |------#|                 R1 type starts at next ts
//      |----------------|           given comment-start position
//      |----------------#------| R3 adjusted comment-start position
//......A.......B.......C.......D.......E.......F.......G.......H...
// tw=2
type sn struct { //             /< adjusted
  field   int             // midlen 
  n       int64           // short
  longlongname string     // R7 for type
}
// tw=4
type sn struct { //             /< adjusted
    field   int             // midlen 
    n       int64           // short
    longlongname string     // R7 for type
}
// tw=8
type sn struct { //             /< adjusted
        field   int             // midlen 
        n       int64           // short
        longlongname string     // R7 for type
}

Caveat: If the block opening brace changes position pre or past its current tabstop span (eg. if a struct was renamed), whole block will make to the changeset. These changes though are less likely than changes to the inner structure.

Rationale

As explained in the background. This change will benefit Go programmers that use source control. Ie. likely all of them.

Compatibility

This is an opt-in feature. It does not affect already written code.

Implementation

None yet. It can successfully be implemented only by the core team.
(Because such a change makes sense if implemented across the whole Go ecosystem.)

Open issues

  1. Should cs-mark on inner blocks be ignored or respected? Eg. marks may legitimately get there due to wrapping.
  2. Can composite literals benefit from tsPos

Edits

  1. 2020/02/24 make R4 wording consistent
@toothrot toothrot added the Proposal label Feb 19, 2020
@toothrot toothrot added this to the Proposal milestone Feb 19, 2020
@toothrot toothrot added this to Incoming in Proposals Feb 19, 2020
@velovix
Copy link

@velovix velovix commented Feb 23, 2020

My first impression is that this proposal adds a fair bit of complexity to gofmt without a whole lot of benefit. I don't believe there exists any other mechanisms like this that allow you to influence gofmt's output, and I see that as mostly a good thing.

When I write a struct with many fields, I often find myself dividing them up into logical sections. Since sections of fields with a blank line or comment in between are grouped independently of each other, I don't end up with massive diffs every time I add or change a field name. This isn't something I do to appease gofmt, it's just a way I find helps me organize the fields.

type ClientConfig struct {
	// Connection info
	hostname   string
	port       int
	scheme     string
	retryCount int

	// Authentication
	username string
	password string
}

I think it's also worth mentioning that it is possible in most cases to hide whitespace-only changes when diffing. For Git, that's git diff -w. GitHub has an option to hide whitespace changes under a settings gear icon in the "Files Changed" view.

I'm not saying there isn't a problem here. I've certainly seen my fair share of Go diffs that are messier than they need to be because of this. However, I think it will be difficult to find a change to gofmt whose complexity or disruptiveness can be justified to solve what I find to be no more than a minor annoyance. For me, this proposal doesn't meet that bar.

@ohir ohir changed the title cmd/gofmt: proposal: Minimize adjacent whitespace changes on struct layout edits. cmd/gofmt: proposal: Minimize adjacent whitespace changes on struct layout edits (user controlled positions). Feb 24, 2020
@ohir
Copy link
Author

@ohir ohir commented Feb 24, 2020

Remarks regarding allowing user-settable format are valid. I reopened proposal using less controversial solution as #37399

@ohir ohir closed this Feb 24, 2020
@ohir
Copy link
Author

@ohir ohir commented Feb 24, 2020

@velovix

I think it's also worth mentioning that it is possible in most cases to hide whitespace-only changes when diffing.

Not only humans do diffs. This proposal was about making changesets smaller in the repo, so to have changeset truly reflecting changes made by a human author.

I'm not saying there isn't a problem here. [...]
I think it will be difficult to find a change to gofmt whose complexity or disruptiveness can be justified to solve what I find to be no more than a minor annoyance.

Please see the simplified (37399) proposal.

@rsc rsc moved this from Incoming to Declined in Proposals Mar 4, 2020
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
3 participants
You can’t perform that action at this time.