Use go modules, analysis package and add test #8

bombsimon · 2023-08-10T21:26:22Z

Use go modules
Use analysis.Analyzer for the linter
Introduce running mode to use from golangci-lint
Add tests using the testing package and assert fixer
Add standalone linter (cmd/whitespace)

Sorry for the big PR and rewrite! While doing some refactoring I renamed main.go to whitespace.go (since it's not a main package) but it seems like git doesn't realize it's a rename as well so the whole file is diffing. As you can see most of the original code is intact and the changes are described below.

Given the current requirements in golangci-lint and per the discussion in golangci/golangci-lint#3967, linters needs to use the analysis package. This PR mostly just adapts to that but to also support fixes from the analyzer itself (which only makes sense since we at one point want to address golangci/golangci-lint#1779). To make the analysis fixer work I did some additional changes:

There's now a difference between where the diagnostic/error will be marked (Message.Diagnostic), where a fix will start which will be the LBrace or following comment (Message.FixStart) and where the first statement which is where the fix will end (Message.FixEnd)
To avoid having golangci-lint to use the cache to read the file and find the line number, I added Message.LineNumber which tells which line number should be deleted when removing whitespaces
MessageType is now slimmed down to only being either MessageTypeRemove or MessageTypeAdd

To allow the run method to return diagnostics in a way golangci-lint want I added support for a running mode which will be set to RunningModeGolangCI when called from golangci-lint.

There's actually no need for this change to add the cmd/whitespace binary but given how simple it is to make an executable that also supports fixing I did so anyway.

Another benefit of using the analysis package is the ease of testing. Given that I added tests for different configuration to ensure the linter does what it's supposed to do.

I already prepared a PR in golangci-lint which will make use of the changes in this PR: golangci/golangci-lint#4003

* Use go modules * Use `analysis.Analyzer` for the linter * Introduce running mode to use from `golangci-lint` * Add tests using the testing package and assert fixer * Add standalone linter (`cmd/whitespace`)

ldez · 2023-08-10T21:48:00Z

It can be a good idea to add GitHub action.

whitespace.go

robinknaapen · 2023-08-28T11:27:14Z

I also found a "issue" in whitespace

When not using gofmt. It's possible to create multiple whitespaces

package main

import "fmt"

func fn3() {
    // A comment
                   // < Whitespace will target this line
                   // < This is not seen by whitespace, thus fix will only omit the previous line, resulting in yet another linter warning?
    fmt.Println(`Hello World`)
}

I don't know if this is important enough to fix. Because majority of people use gofmt...

bombsimon · 2023-08-28T19:46:36Z

I don't know if this is important enough to fix. Because majority of people use gofmt...

This is a great question! And I don't think I can answer this. I discussed it briefly in the golangci-lint PR here: golangci/golangci-lint#4003.

Also as seen in my my comment #8 (comment), applying the suggested fixes with the -fix flag will also reformat the file according to go fmt so if you want to keep your non formatted file it's not possible to use the analyzer. Not sure if that's a blocker for you.

Here's an example package that I formatted, showing other changes as well:

Before with inconsistent spaces:

› bat -A example.go
───────┬─────────────────────────────────────
       │ File: example.go
───────┼─────────────────────────────────────
   1   │ package·x␊
   2   │ ␊
   3   │ import·"fmt"␊
   4   │ ␊
   5   │ func·fn()·{␊
   6   │ ├──┤····␊
   7   │ ····fmt.Println("Hello,·World")␊
   8   │ ␊
   9   │ ········fmt.Println("Hello,·World")␊
  10   │ }␊
───────┴─────────────────────────────────────

And after applying suggested fixes:

› bat -A example.go
───────┬─────────────────────────────────
       │ File: example.go
───────┼─────────────────────────────────
   1   │ package·x␊
   2   │ ␊
   3   │ import·"fmt"␊
   4   │ ␊
   5   │ func·fn()·{␊
   6   │ ├──┤fmt.Println("Hello,·World")␊
   7   │ ␊
   8   │ ├──┤fmt.Println("Hello,·World")␊
   9   │ }␊
───────┴─────────────────────────────────

Personally I have a hard time seeing the target audience for someone that's interested in a formatter like this but don't care for go fmt but I have no data to back that up, just a gut feeling.

To avoid false positives I could rewrite this to actually get the position of where the comment ends instead of the LBrace but otherwise just running the linter twice should fix the problem.

robinknaapen · 2023-08-31T12:23:48Z

@bombsimon

Also as seen in my my comment #8 (comment), applying the suggested fixes with the -fix flag will also reformat the file according to go fmt

Thats good information, I wasn't aware of the fact that the -fix flag also fixes like gofmt
Is this done before or after analysis.

If it's before, than the issue will is non-existent

To avoid false positives I could rewrite this to actually get the position of where the comment ends instead of the LBrace but otherwise just running the linter twice should fix the problem.

This is "beaking" in a logical sense. But I don't think it's an impact at all. I'll ask my colleague for his opinion to this

@elwint, what is your thought on this?

bombsimon · 2023-08-31T15:21:08Z

Is this done before or after analysis.

Sadly it looks like after.

Before:

› bat -A example.go
───────┬───────────────────────────────────────
       │ File: example.go
───────┼───────────────────────────────────────
   1   │ package·main␊
   2   │ ␊
   3   │ ␊
   4   │ ␊
   5   │ ␊
   6   │ import·"fmt"␊
   7   │ ␊
   8   │ func·main()·{␊
   9   │ ␊
  10   │ ␊
  11   │ ␊
  12   │ ····fmt.Println("Hello,·World")␊
  13   │ ␊
  14   │ ····├──┤fmt.Println("Hello,·World·2")␊
  15   │ }␊
───────┴───────────────────────────────────────

After (only one run)

› bat -A example.go
───────┬───────────────────────────────────
       │ File: example.go
───────┼───────────────────────────────────
   1   │ package·main␊
   2   │ ␊
   3   │ import·"fmt"␊
   4   │ ␊
   5   │ func·main()·{␊
   6   │ ␊
   7   │ ├──┤fmt.Println("Hello,·World")␊
   8   │ ␊
   9   │ ├──┤fmt.Println("Hello,·World·2")␊
  10   │ }␊
───────┴───────────────────────────────────

This is actually an issue for golangci-lint as well since I only store one line to remove. Maybe it would actually be petter to store the exact position of the ennd of the last node or comment and the start of the first node or commennt to have the exact range. We could also store a list of lines to remove for golangci-lint. However, the comment linked above (golangci/golangci-lint#4003) will still be an issue although more related to golangci-lint than this linter. I think if we had pos.End() of the last comment or node we could use that one to add a newline and also solve the issue in golangci-lint so that's two reasons to actually rewrite this.

robinknaapen · 2023-09-07T13:03:10Z

@bombsimon

First of all, thanks for being patient.

I think that if it's doable than why not at this point

bombsimon · 2023-09-07T13:38:00Z

First of all, thanks for being patient.

Thank you for taking the time to review this and considering the PR 😃

I think that if it's doable than why not at this point

Sure thing, I'll give this a go when I get the time!

Instead of relying on the file being `gofmt`:ed and only containing one empty line at most this commit will keep track of comments after the opening bracket of block statements and set the fix start to where this comment ends. This means that we will fix all the way from the left bracket or comment til the first statement no matter how many empty lines this is. This will also keep a list of lines affected by the linter to support fixing multiple empty lines in `golangci-lint`.

bombsimon · 2023-10-03T20:38:35Z

Wow, can't believe this took me almost a month, sorry about that!

Anyway, I added a change so we now keep track of where the opening position is; either the left bracket or the end of a comment on the same line. This made me able to set the whole fix range between where this node ends and where the first statement (or comment) starts.

While doing this I also changed LineNumber to LineNumbers so we can fix the whole range in golangci-lint as well and not only a single line.

I added some test cases where the file is not formatted and contains multiple lines. Sadly since the analysistest package requires the // want directive I cannot write a unit test (easily) that checks this works without comments but I just made a simple test file locally:

package main

import "fmt"

func main() {


    fmt.Println("Hello, World")


}

And ran go run ./cmd/whitespace -fix ./testpkg and it formatted the file correctly.

testdata/src/whitespace_no_multiline/no_multiline.go

whitespace.go

testdata/src/whitespace_no_multiline/no_multiline.go

robinknaapen · 2023-10-04T08:44:00Z

@bombsimon

Wow, can't believe this took me almost a month, sorry about that!

Don't mention it, I really appreciate your expertise. Thanks for all the effort 💪

robinknaapen · 2023-10-30T08:03:05Z

@bombsimon

Sorry for the delay.

Yes, currently it does. But I think it's just a side effect and not intentional. In fact, I think the current check for comments is wrong, this line should check if the comment starts on the same line, not if it ends.

The consensus about this case is that it should indeed be allowed.

After this I think that's all

robinknaapen · 2023-10-30T21:36:50Z

@bombsimon

Nice solution 💪

First thing in the morning I will review and ask a colleague for review.

Thanks for all the effort once again

bombsimon · 2023-10-30T21:39:09Z

Sorry for the delay.

No worries, there's no rush here and I appreciate you're considering this PR.

Nice solution 💪

First thing in the morning I will review and ask a colleague for review.

Thanks! Yeah the change grew over time and I understand that this is a lot to review. But I hope that all the test cases should help add some confidence that the code is actually doing what it's supposed to do. If you can think of any more test cases that would probe the expected behaviour feel free to add them!

Thanks for all the effort once again

Thank's for the great feedback and considering this PR! Just let me know if there's anything else missing or that needs clarification.

ldez · 2023-11-02T13:44:11Z

Can you create a tag? (I suggest v0.1.0)

robinknaapen · 2023-11-02T15:06:02Z

@ldez

Yes, I will. Thanks for reminding 😅

Use go modules, analysis package and add test

19b2d28

* Use go modules * Use `analysis.Analyzer` for the linter * Introduce running mode to use from `golangci-lint` * Add tests using the testing package and assert fixer * Add standalone linter (`cmd/whitespace`)

bombsimon mentioned this pull request Aug 10, 2023

whitespace: update after moving to the analysis package golangci/golangci-lint#4003

Merged

bombsimon added 2 commits August 11, 2023 23:14

Fix comments and typos, add another test

e334819

Add GitHub actions

6064ab3

robinknaapen reviewed Aug 28, 2023

View reviewed changes

whitespace.go Show resolved Hide resolved

robinknaapen reviewed Oct 4, 2023

View reviewed changes

testdata/src/whitespace_no_multiline/no_multiline.go Outdated Show resolved Hide resolved

whitespace.go Outdated Show resolved Hide resolved

testdata/src/whitespace_no_multiline/no_multiline.go Show resolved Hide resolved

Fix typos, add tests from PR

b5ade39

Multiline comments ending after opening pos is ok

4f3cb80

robinknaapen approved these changes Oct 31, 2023

View reviewed changes

Fix typo

7f92e17

robinknaapen requested review from robinknaapen and UltraBwko November 1, 2023 08:18

robinknaapen approved these changes Nov 1, 2023

View reviewed changes

elwint approved these changes Nov 2, 2023

View reviewed changes

elwint merged commit e527b27 into ultraware:master Nov 2, 2023

bombsimon deleted the use-analysis branch November 2, 2023 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use go modules, analysis package and add test #8

Use go modules, analysis package and add test #8

bombsimon commented Aug 10, 2023 •

edited

Loading

ldez commented Aug 10, 2023

robinknaapen commented Aug 28, 2023

bombsimon commented Aug 28, 2023

robinknaapen commented Aug 31, 2023

bombsimon commented Aug 31, 2023 •

edited

Loading

robinknaapen commented Sep 7, 2023

bombsimon commented Sep 7, 2023

bombsimon commented Oct 3, 2023

robinknaapen commented Oct 4, 2023

robinknaapen commented Oct 30, 2023

robinknaapen commented Oct 30, 2023

bombsimon commented Oct 30, 2023

ldez commented Nov 2, 2023

robinknaapen commented Nov 2, 2023

Use go modules, analysis package and add test #8

Use go modules, analysis package and add test #8

Conversation

bombsimon commented Aug 10, 2023 • edited Loading

ldez commented Aug 10, 2023

robinknaapen commented Aug 28, 2023

bombsimon commented Aug 28, 2023

robinknaapen commented Aug 31, 2023

bombsimon commented Aug 31, 2023 • edited Loading

robinknaapen commented Sep 7, 2023

bombsimon commented Sep 7, 2023

bombsimon commented Oct 3, 2023

robinknaapen commented Oct 4, 2023

robinknaapen commented Oct 30, 2023

robinknaapen commented Oct 30, 2023

bombsimon commented Oct 30, 2023

ldez commented Nov 2, 2023

robinknaapen commented Nov 2, 2023

bombsimon commented Aug 10, 2023 •

edited

Loading

bombsimon commented Aug 31, 2023 •

edited

Loading