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: reformats block comments #5128

Open
gopherbot opened this issue Mar 25, 2013 · 24 comments
Open

cmd/gofmt: reformats block comments #5128

gopherbot opened this issue Mar 25, 2013 · 24 comments
Assignees
Labels
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 25, 2013

by borman@google.com:

What steps will reproduce the problem?

Use /* */ for block comments.  Example:

func f() {
        /*
         * don't do this right now
        fmt.Printf(`a
multi-line
string`)
        */
        return
}

Gofmt alters the content of the comment (which includes the /* and */).  The first time
gofmt is called it produces:

func f() {
        /*
                 * don't do this right now
                fmt.Printf(`a
        multi-line
        string`)
        */
        return
}

and if called again it produces:

func f() {
        /*
                         * don't do this right now
                        fmt.Printf(`a
                multi-line
                string`)
        */
        return
}

gofmt should not have altered the comment in the first place.  The /* was indented
correctly up front.
@gopherbot
Copy link
Author

@gopherbot gopherbot commented Mar 26, 2013

Comment 1 by borman@google.com:

Another indentation fail with /* */:
Start with:
/*
        This is a comment
        It is indented with 8 spaces.
        And has several lines
 */
Gofmt changes this to:
/*
   This is a comment
   It is indented with 8 spaces.
   And has several lines
*/
It has changed my 8 spaces into 3.
@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 3, 2013

Comment 2:

Owner changed to @griesemer.

@gopherbot
Copy link
Author

@gopherbot gopherbot commented May 26, 2013

Comment 3 by hamo.by:

The first issue you reported seems a expected output for issue #1835.
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 26, 2013

Comment 4:

There is a tension between gofmt leaving comments alone and "adjusting" them to match
surrounding code. There is a know bug where formatting of /* comments is not idempotent
(as witnessed in the first example above), and where formatting doesn't change anymore
after two applications of gofmt (as is also the case in the first example above).
Being completely idempotent is a very hard problem (short of simply running gofmt twice
internally).
Independent of idempotency, the question is what gofmt should be doing with /* comments.
You are suggesting that it should leave it alone if the first line is indented
correctly. Perhaps that is the right answer. But I suspect there are counter examples.
Leaving alone for now.

Status changed to Thinking.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 5:

Labels changed: added priority-later, go1.2maybe, removed priority-triage.

@rsc
Copy link
Contributor

@rsc rsc commented Jul 30, 2013

Comment 6:

Labels changed: added feature.

@robpike
Copy link
Contributor

@robpike robpike commented Aug 16, 2013

Comment 7:

Leaving alone for 1.2.

Labels changed: added go1.3maybe, removed go1.2maybe.

@robpike
Copy link
Contributor

@robpike robpike commented Aug 20, 2013

Comment 8:

Labels changed: removed go1.3maybe.

@josharian
Copy link
Contributor

@josharian josharian commented Sep 18, 2013

Comment 9:

Another use for leaving /* comments */ completely untouched is for example output with
whitespace: https://golang.org/issue/6416
@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 10:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 11:

Labels changed: removed feature.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 13:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 14:

Labels changed: added repo-main.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 19, 2014

Comment 15:

I ran into this issue recently, with an even simpler reproduce case:
http://play.golang.org/p/Pwn4cOHtMN
@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 19, 2014

Comment 16:

Cool! Thanks.
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 2, 2015

I've done some investigating here.

I ran into this issue recently, with an even simpler reproduce case:

http://play.golang.org/p/Pwn4cOHtMN

That particular issue is due to an unhandled edge case in stripCommonPrefix of go/printer.

It happens when a comment block consists of a first line with /*, 1 or more blank lines, last line with */. The internal prefix variable ends up never being set to a correct value before being used.

Handling it correctly will fix that particular sample I provided (http://play.golang.org/p/Pwn4cOHtMN), but it seems like it's not the same issue as what's described in the original post.

Should I break http://play.golang.org/p/Pwn4cOHtMN out into a separate issue then? Or just consider this current issue to be it?

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 2, 2015

Okay, I've created failing test cases in shurcooL/play@bfc69a3 and a simple fix for the issue in shurcooL/play@f416cec. I didn't try to refactor it in any way.

I can submit a CL for review as soon as I'm sure if I should say "fixes #5128" or create a separate issue...

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Feb 2, 2015

After re-reading the original issue and the responses it received, I feel that I should move http://play.golang.org/p/Pwn4cOHtMN into a separate, much smaller issue. That way I can proceed to make a CL that completely fixes and closes it. So, I've created #9751.

It has a simple and direct fix. I've made a CL for it.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@c0b
Copy link

@c0b c0b commented May 16, 2017

I have an Example test case:

func ExampleDecode() {
	call-my-test-case it print some output

	// Output:
	/*
here is the output in a block comment
   in multi lines with different indentation
	*/
}

The test case is passing, it's no problem

but when I run go fmt, it constantly change the block comment to this style

	/*
	here is the output in a block comment
	   in multi lines with different indentation
	*/

and causing test case to FAIL;

wonder is it the same case here, is there any way to let go fmt not touching my block comment?

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 16, 2017

@c0b There's no way to let go fmt know that it should leave comments alone. In your case, try the use of // comments for the Example output instead (e.g., like in https://golang.org/src/text/scanner/example_test.go).

A future gofmt (version) probably should not touch comments at all, but that has other issues.

@vearutop
Copy link
Contributor

@vearutop vearutop commented Nov 6, 2018

It is a real pain to work with multi-line // output: in example tests. I'm trying to assert YAML value.

  • /* ... */ gets broken by gofmt.
  • // per-line is unmanageable, I use Goland to comment multiple lines with hotkey, but it does not add space between // and line content.

Currently I have to paste expected content, add // with hotkey and then replace // to // to make things work.

I would appreciate that gofmt does not touch comments following // output:.

@c0b
Copy link

@c0b c0b commented Jan 3, 2019

There's no way to let go fmt know that it should leave comments alone

how about to change go fmt to recognize the block comment followed by keywords like Output: this convention is in-use by go test; should be considered a builtin feature of go?

there might be other keywords should be recognized as well, if go fmt doesn't want to recognize all of them, then may create a special comment marker like gofmt: DO NOT TOUCH in the first line of the block comment?

A future gofmt (version) probably should not touch comments at all, but that has other issues.

@griesemer are you aware of any ongoing effort of this future gofmt? can connect related PR to here?

@komuw
Copy link
Contributor

@komuw komuw commented Oct 15, 2020

I just spent 1 hour trying to get gofmt to leave alone the // output: of a test example I was working on(#41980), to no avail; until I searched and came across this issue.

I feel like gofmt re-formatting block comments in the specific case of // output: of a test example should be a separate issue that should be decided on independent of this one. But maybe this is just my frustration showing.

@komuw
Copy link
Contributor

@komuw komuw commented Oct 15, 2020

It seems impossible, at least for me, to get the following reproducer test to pass; if you run gofmt before running go test. Because gofmt will always reformat the // output comment. And hence this sounds like a bug to me rather than a feature request.

package main

import (
	"fmt"
)

func Hello() {
	fmt.Printf("Hello \n\nWorld")
}

// 1. https://golang.org/pkg/testing/#hdr-Examples
// 2. https://blog.golang.org/examples
func ExampleHello() {
	Hello()

	// Output:
	// Hello 
	//
	// World
}
komuw added a commit to komuw/kama that referenced this issue Oct 15, 2020
What:
- Add example test. 
That test example is kind of blocked on;
1. golang/go#41980       
2. golang/go#5128 (comment)
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
9 participants
You can’t perform that action at this time.