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

Add WSL linter #771

Merged
merged 6 commits into from
Oct 4, 2019
Merged

Add WSL linter #771

merged 6 commits into from
Oct 4, 2019

Conversation

bombsimon
Copy link
Member

Added linter WSL. Tests and linter is working with golangci-lint run, however I'm experiencing an old issue noted in #255.

This is with golangci-lint built from this commit (master + 08de1f8) and goimports installed with go get today (GO111MODULE=off go get -u golang.org/x/tools/cmd/goimports). No difference with -local flag or not.

golangci-lint run pkg/golinters/wsl.go
pkg/golinters/wsl.go:6: File is not `goimports`-ed (goimports)
        "github.com/bombsimon/wsl"

goimports -w pkg/golinters/wsl.go
gofmt -w -s pkg/golinters/wsl.go

diff \
  <(goimports pkg/golinters/wsl.go) \
  <(cat pkg/golinters/wsl.go) | wc -l
       0

golangci-lint run pkg/golinters/wsl.go
pkg/golinters/wsl.go:6: File is not `goimports`-ed (goimports)
        "github.com/bombsimon/wsl"

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2019

CLA assistant check
All committers have signed the CLA.

@tpounds tpounds added the linter: new Support new linter label Oct 2, 2019
@tpounds
Copy link
Contributor

tpounds commented Oct 2, 2019

@bombsimon Thanks for the submission. How does your WSL linter differ from https://github.com/ultraware/whitespace which was recently added?

@bombsimon
Copy link
Member Author

bombsimon commented Oct 2, 2019

@tpounds WSL has a bunch more features other than checking leading and trailing whitespaces in blocks. It checks for variable usages and different node types to determine if there should be a newline or not. Please give the WSL readme a quick look to get a better understanding of how WSL works, there should be a lot of examples describing how the linter works.

EDIT: You could say that the main purpose of WSL is to add whitespaces to not make the code too tight whereas whitespace encourage you to remove whitespaces (altough WSL does both).

@bombsimon
Copy link
Member Author

Other than the goimports issue I see I have a problem with go mod and go1.13. Since I've had some problems with go mod I've set GOSUMDB=off in my go environment and thus didn't notice the issue with my package.

I can't believe I'm having such ha hard time getting go mod to work, I feel stupid.

I've added a tag v1.0.0 to master and I've created a release named v1.0.0. Apparently this is not enough and result in go mod not working:

go: github.com/bombsimon/[email protected]: unexpected status (https://proxy.golang.org/github.com/bombsimon/wsl/@v/v1.0.0.info): 410 Gone

Doesn't seem to ba any problem when trying said URL manually:

http https://proxy.golang.org/github.com/bombsimon/wsl/@v/v1.0.0.info
HTTP/1.1 200 OK
Accept-Ranges: bytes
Age: 64
Alt-Svc: quic=":443"; ma=2592000; v="46,43",h3-Q048=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000
Cache-Control: public, max-age=10800
Content-Length: 50
Content-Type: application/json
Date: Wed, 02 Oct 2019 14:50:55 GMT
Expires: Wed, 02 Oct 2019 17:50:55 GMT
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

{
    "Time": "2019-10-02T13:06:13Z",
    "Version": "v1.0.0"
}

GOPROXY and GOSUMDB are set to "" in the Travis CI environment and if I do the same in a new project and run go get -u github.com/bombsimon/[email protected] everything is working.

Any help or pointers in the right direction is much appreciated!

@@ -208,6 +208,10 @@ linters-settings:
whitespace:
multi-if: false

wsl:
# Skip test files
no-test: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. please, don't use negative naming
  2. please, don't add such duplicating option: any linter can be disabled for tests by exclude-rules. It's not performance effective, I agree, but it should be done in a general way for all linters, not per-linter

Copy link
Member Author

@bombsimon bombsimon Oct 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, I'll remove this since the configuration isn't needed!

pkg/golinters/wsl.go Show resolved Hide resolved
@bombsimon
Copy link
Member Author

Ok so now the Travis pipeline fails with the lint error File is not goimports-ed (from goimports). I have no idea how I should solve that, see my previous comments about what I've tried with goimports [-local] and gofmt -s. Any ideas how I could resolve this?

@jirfag
Copy link
Member

jirfag commented Oct 4, 2019

@bombsimon did you update your goimports?
also, you can fix it by committing suggestion by golangcibot or by running golangci-lint run --fix

@bombsimon
Copy link
Member Author

@jirfag Just fixed them with golangci-lint instead of goimports itself. Don't know why goimports -w doesn't fix the issue but I guess I'm running a bad version. goimports -w doesn't make any changes before or after the fix with golangci-lint.

Seems like GolangCI is happy now at least! :)

Copy link
Contributor

@tpounds tpounds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bombsimon Can you add some tests that verify the linter's behavior? This will ensure any regressions will be caught when we upgrade linter versions, add/remove configuration options, and/or tweak internal.

e.g.

https://github.com/golangci/golangci-lint/blob/master/test/testdata/dogsled.go
https://github.com/golangci/golangci-lint/blob/master/test/testdata/errcheck.go
https://github.com/golangci/golangci-lint/blob/master/test/testdata/gocognit.go

Edit: Looks like I missed that you added a single test. What do you think about expanding that to more of the checks?

@bombsimon
Copy link
Member Author

@tpounds Absolutely, will add more tests!

@tpounds
Copy link
Contributor

tpounds commented Oct 4, 2019

@bombsimon Thanks for the additional tests! It looks like CI is still failing due to a go.mod/go.sum mismatch.

https://travis-ci.com/golangci/golangci-lint/jobs/242161917#L699

Can you run go mod tidy and/or make vendor to fix this issue?

Copy link
Contributor

@tpounds tpounds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bombsimon Nice! Thanks for the contribution! 🚢

@tpounds tpounds merged commit 3e09174 into golangci:master Oct 4, 2019
@bombsimon
Copy link
Member Author

@tpounds Thank you for your feedback, help and the merge! What's common practice regarding changes (new errors, new/changed line markings etc)? Should I bump major or minor if/when I do changes to those? It's not really a common interface/API used by other's so I might as well ask. Anything else I should think about now that it's a part of golangci-lint?

@tpounds
Copy link
Contributor

tpounds commented Oct 5, 2019

@bombsimon In general I think following standard semantic versioning is reasonable. In the case of third party linters it's fairly inconsistent across the board. That said, I think these are general interpretations to the semver guidlines that seem reasonable to me:

  1. Bump major version when you add a new detection rule that is on by default or flip an existing detection rule from on to off or off to on.
  2. Bump minor version when you add a new detection rule that is off by default but can be opted into.
  3. Bump patch version when you fix a detection bug (e.g. fix false positives/negatives), update deps, etc.

/cc @jirfag

stevendanna added a commit to chef/ci-studio-common that referenced this pull request Oct 23, 2019
The newest golangci-lint added a new linter `wsl`:

   golangci/golangci-lint#771

The linting rules it adds don't seem particularly useful to me.

Signed-off-by: Steven Danna <[email protected]>
@tpounds tpounds mentioned this pull request Nov 8, 2019
theckman pushed a commit to theckman/golangci-lint that referenced this pull request May 3, 2020
$ git cherry --abbrev -v 0af0999fabfb ee9bf5809ead
+ abd8436 all: enable Go modules on CI (golangci#753)
+ 3c9d0fb checkers: recognize //line and //nolint in commentFormatting (golangci#756)
+ 0b517d7 checkers: extend deprecatedComment patterns (golangci#757)
+ 09100f6 checkers: use astcast package instead of coerce.go (golangci#758)
+ 2e9e97f checker: simplify boolExprSimplify (golangci#759)
+ 575701e make: add go-consistent to CI checks list (golangci#761)
+ b55f431 checkers: fix unlambda handling of builtins (golangci#763)
+ 5a7dee3 checker: handle lambdas properly in boolExprSimplify (golangci#765)
+ 5ce3939 checkers: teach boolExprSimplify a few new tricks (golangci#766)
+ 04d160f checkers: add new patterns to boolExprSimplify (golangci#768)
+ 09582e2 make: collect coverprofile separately from goveralls (golangci#769)
+ d8d0ee4 checkers: recognize NOTE pattern in deprecatedComment (golangci#770)
+ 12f0f85 Update copyright notice to 2019 (golangci#771)
+ f54bdb6 checkers: add stringXbytes checker
+ 170d65c checkers: followup for golangci#773 (golangci#774)
+ 84e9e83 checkers: make stringXbytes more linear (golangci#775)
+ a800815 checkers: add Depreacted typo pattern (golangci#776)
+ 6751be9 checkers: add hexLiterals (golangci#772)
+ ac61906 checkers: add typeAssertChain checker (golangci#782)
+ d19dbf1 checkers: add codegenComment checker (golangci#783)
+ d82b576 checkers: proper pkg/obj check for flagName (golangci#786)
+ dfcf754 ci: enable integration tests (golangci#787)
+ 5dafc45 checkers: fix equalFold false positive (golangci#788)
+ ed5e8e7 checkers: refactor and fix hexLiteral checker (golangci#789)
+ e704e07 checkers: add argOrder checker (golangci#790)
+ 34c1dc8 checkers: add Split handling to argOrder checker (golangci#791)
+ cbe095d checkers: add math.Max and math.Min to dupArg (golangci#792)
+ c986ee5 checkers: add checkers info fields test (golangci#794)
+ 66e5832 cmd/makedocs: use lintpack, fix build (golangci#793)
+ 6bce9d0 cmd/makedocs: add enabled/disabled by default info (golangci#795)
+ 4adbf9a checkers: simplify flagName (golangci#799)
+ 07de34a checkers: add octalLiteral checker (golangci#798)
+ 765907a cmd/makedocs: add checker param docs (golangci#796)
+ ee9bf58 cmd/makedocs: fix headers formatting (golangci#803)
@ldez ldez added this to the v1.20 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants