Enforce leading space on comments #5747
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# What
Changes all comments like this:
```go
//stuff
type thing struct {
field string //comment
}
//go:embed directives are left alone
```
To this:
```go
// stuff
type thing struct {
field string // comment
}
//go:embed directives are left alone
```
And added a fairly basic lint check to prevent them from coming back.
The lint check is imprecise and doesn't catch comments at the ends of non-blank lines of code,
but fixing that seemed like much more work than it was worth.
And, since this will likely neutralize a `//nolint` comment that wasn't doing anything anyway,
I've removed that one comment entirely to avoid being misleading.
# How
"Starts with X but not followed by Y" is famously hard for regex, so I only tried briefly with sed / perl before giving up.
So instead I just had Goland format the entire repository, and undid anything that wasn't a Go file.
Somewhat surprisingly, all it did was add these spaces. Which is perfect.
# Why
IDEs often do this when formatting files, which is thrashing a bit.
gofmt (and therefore goimports) in theory doesn't actually care... but despite their protestations
about comment formatting in github issues, gofmt *does* treat some comments specially,
namely "directives" like `go:build`:
```diff
diff --git cmd/server/main.go cmd/server/main.go
index 6af56db58..b857f9211 100644
--- cmd/server/main.go
+++ cmd/server/main.go
@@ -35,6 +35,14 @@ import (
)
// main entry point for the cadence server
+//go:build
+// did you know
+//go:link
+// that go special
+//cases:things
+// that use
+//go:embed
+// specific comment structures?
func main() {
app := cadence.BuildCLI(metrics.ReleaseVersion, metrics.Revision)
app.Run(os.Args)
```
which `gofmt -w` turns into:
```diff
diff --git cmd/server/main.go cmd/server/main.go
index 6af56db58..2511eb414 100644
--- cmd/server/main.go
+++ cmd/server/main.go
@@ -18,6 +18,8 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
+//go:build
+
package main
import (
@@ -35,6 +37,14 @@ import (
)
// main entry point for the cadence server
+// did you know
+// that go special
+// that use
+// specific comment structures?
+//
+//go:link
+//cases:things
+//go:embed
func main() {
app := cadence.BuildCLI(metrics.ReleaseVersion, metrics.Revision)
app.Run(os.Args)
```
which IMO means we should explicitly avoid colliding with those kinds of comment patterns.
Linters like staticcheck generally use suppressing-comment structures like `//nolint:sa123`,
I suspect in part to fit with / avoid running afoul of this kind of reformatting.
Go has only claimed `x:y` as far as I can tell, so specifically `x:` is not a "directive"...
but that seems unnecessarily near-colliding to me, and it would allow `//TODO: asdf` which
are definitely not "tool directives" in any form, so I haven't allowed those.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov Report
Additional details and impacted files
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
davidporter-id-au
approved these changes
Mar 7, 2024
taylanisikdemir
approved these changes
Mar 7, 2024
Groxx
commented
Mar 7, 2024
| @@ -36,7 +36,6 @@ type ( | |||
|
|
|||
| // metricDefinition contains the definition for a metric | |||
| metricDefinition struct { | |||
| //nolint | |||
There was a problem hiding this comment.
this is the one being removed.
removing it doesn't cause any lints to fail, and it doesn't really make sense here anyway, so I'm just getting rid of it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Changes all comments like this:
To this:
And added a fairly basic lint check to prevent them from coming back.
The lint check is imprecise and doesn't catch comments at the ends of non-blank lines of code,
but fixing that seemed like much more work than it was worth.
And, since this will likely neutralize a
//nolintcomment that wasn't doing anything anyway,I've removed that one comment entirely to avoid being misleading.
How
"Starts with X but not followed by Y" is famously hard for regex, so I only tried briefly with sed / perl before giving up.
So instead I just had Goland format the entire repository, and undid anything that wasn't a Go file.
Somewhat surprisingly, all it did was add these spaces. Which is perfect.
Oddly it missed a single obvious one, but I'm willing to forgive it for that.
Why
IDEs often do this when formatting files, which is thrashing a bit.
gofmt (and therefore goimports) in theory doesn't actually care... but despite their
protestations about not caring about comment formatting in github issues, Go's
source-printer and therefore gofmt does treat some comments specially, most notably
"directives" like
go:build: https://cs.opensource.google/go/go/+/refs/tags/go1.22.1:src/go/printer/comment.go;l=15and anything that might be a "doc comment" may get restructured too:
which
gofmt -wturns into:which IMO means we should explicitly avoid colliding with those kinds of comment patterns.
Linters like staticcheck generally use suppressing-comment structures like
//nolint:sa123,I suspect in part to fit with / avoid running afoul of this kind of reformatting.
Go has only claimed
x:yas far as I can tell, so specificallyx:is not a "directive"...but that seems unnecessarily near-colliding to me, and it would allow
//TODO: asdfwhichare definitely not "tool directives" in any form, so I haven't allowed those.