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

x/tools/gopls: don't rename documentation in compiler directive comments #42331

Closed
zikaeroh opened this issue Nov 2, 2020 · 7 comments
Closed

x/tools/gopls: don't rename documentation in compiler directive comments #42331

zikaeroh opened this issue Nov 2, 2020 · 7 comments
Assignees
Milestone

Comments

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Nov 2, 2020

What did you do?

I'm testing out the new embed package. I wrote:

//go:embed static/*
var static embed.FS

Then I went to rename the variable to export it to test somewhere else.

What did you expect to see?

The variable is renamed; the comment is left unmodified.

What did you see instead?

I'm left with:

//go:embed Static/*
var Static embed.FS

(Any name works, static -> static2 works just the same.)

Build info

golang.org/x/tools/gopls master
    golang.org/x/tools/gopls@(devel)
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/google/go-cmp@v0.5.1 h1:JFrFEBb2xKufg6XkJsJr+WbKb4FQlURi5RUcBveYu9k=
    github.com/sergi/go-diff@v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
    golang.org/x/mod@v0.3.0 h1:RM4zey1++hCTbCVQfnWeKs9/IEsaBLA8vTkd0WVtmH4=
    golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9 h1:SQFwaSi55rU7vdNs9Yr0Z324VNlrF+0wMqRXT4St8ck=
    golang.org/x/tools@v0.0.0-20201021214918-23787c007979 => ../
    golang.org/x/xerrors@v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
    honnef.co/go/tools@v0.0.1-2020.1.6 h1:W18jzjh8mfPez+AwGLxmOImucz/IFjpNlrKVnaj2YVc=
    mvdan.cc/gofumpt@v0.0.0-20200927160801-5bfeb2e70dd6 h1:z+/YqapuV7VZPvBb3GYmuEJbA88M3PFUxaHilHYVCpQ=
    mvdan.cc/xurls/v2@v2.2.0 h1:NSZPykBXJFCetGZykLAxaL6SIpvbVy/UFEniIfHAa8A=
@gopherbot gopherbot added this to the Unreleased milestone Nov 2, 2020
@stamblerre stamblerre modified the milestones: Unreleased, gopls/unplanned Nov 2, 2020
@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 2, 2020

This is the logic in rename that is meant to update doc comments.
I'm not familiar with the embed package--is there any case when you would want the rename to update the comment?

@zikaeroh
Copy link
Contributor Author

@zikaeroh zikaeroh commented Nov 2, 2020

See #41191. These are file paths/globs on disk that are embedded at compile time into the binary.

@zikaeroh
Copy link
Contributor Author

@zikaeroh zikaeroh commented Nov 2, 2020

I don't think there's a reason to do any renames in a //go:embed comment, though I'd almost argue that any compiler directive should be ignored for renames. // +build comments are getting deprecated in favor of //go:build, so that might cause oddities if you accidentally rename a build tag that's the same as a variable name.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 2, 2020

Thanks! It does sound like we can skip renaming in any comment that starts with //go:.

@stamblerre stamblerre changed the title x/tools/gopls: renaming embed.FS variable renames path in directive x/tools/gopls: don't rename documentation in compiler directive comments Nov 2, 2020
@zikaeroh
Copy link
Contributor Author

@zikaeroh zikaeroh commented Nov 2, 2020

I would suggest matching the pattern given in #37974 / CL 224737, which is how go doc determines which comment lines to ignore. (See "isDirective")

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Nov 2, 2020

Thank you for linking that--will do!

@stamblerre stamblerre self-assigned this Nov 3, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 3, 2020

Change https://golang.org/cl/267121 mentions this issue: internal/lsp: do not rename in compiler directive comments

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