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

Go linting adjustments #13709

Closed
wants to merge 7 commits into from
Closed

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Nov 26, 2020

Seems a major source of linter slowness is that it ventures into directories not containing any go files. Fix that by excluding the worst offending dirs.

Seems to major source of linter slowness is that it ventures into
directories not containing any go files. Fix that by excluding the worst
offending dirs.
@techknowlogick techknowlogick added topic/code-linting skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Nov 26, 2020
@techknowlogick techknowlogick added this to the 1.14.0 milestone Nov 26, 2020
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 26, 2020
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

could you also include an upgrade to golangci-lint v1.33.0 in this PR?

@techknowlogick techknowlogick changed the title Speed up golanglint-ci Speed up golangci-lint Nov 26, 2020
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Perhaps also excluding node_modules from revive as well would decrease time to complete linting.

@silverwind
Copy link
Member Author

silverwind commented Nov 26, 2020

Actually it does not seem to help much if any for golangci-lint but revive certainly seems to digs into node_modules as seen by these random errors on CI:

GO111MODULE=on go run -mod=vendor build/lint.go -config .revive.toml -exclude=./vendor/... ./... \|\| exit 1
pattern ./...: lstat node_modules/bindings: no such file or directory

I'll see if I can fix that, marking wip.

@silverwind silverwind marked this pull request as draft November 26, 2020 22:26
@silverwind
Copy link
Member Author

silverwind commented Nov 26, 2020

Interestingly I get a bunch of new revive errors if I explicitely specify files to it using $(GO_SOURCES_OWN):

modules/setting/database_sqlite.go:10:2: a blank import should be only in a main or test package, or have a comment justifying it
modules/public/static.go:22:1: exported function Asset should have comment or be unexported
modules/public/static.go:31:1: exported function AssetNames should have comment or be unexported
modules/public/static.go:40:1: exported function AssetIsDir should have comment or be unexported
modules/public/static.go:43:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
modules/public/static.go:47:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
modules/templates/static.go:49:1: exported function NewTemplateFileSystem should have comment or be unexported
modules/templates/static.go:206:1: exported function Asset should have comment or be unexported
modules/templates/static.go:215:1: exported function AssetNames should have comment or be unexported
modules/templates/static.go:224:1: exported function AssetIsDir should have comment or be unexported
modules/templates/static.go:227:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
modules/templates/static.go:231:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
modules/templates/static.go:49:30: exported func NewTemplateFileSystem returns unexported type templates.templateFileSystem, which can be annoying to use
build/generate-gitignores.go:26:3: var githubApiToken should be githubAPIToken
modules/options/static.go:56:1: exported function AssetDir should have comment or be unexported
modules/options/static.go:116:1: exported function Asset should have comment or be unexported
modules/options/static.go:125:1: exported function AssetNames should have comment or be unexported
modules/options/static.go:134:1: exported function AssetIsDir should have comment or be unexported
modules/options/static.go:137:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
modules/options/static.go:141:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
build/generate-licenses.go:26:3: var githubApiToken should be githubAPIToken
cmd/embedded.go:327:10: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
cmd/embedded.go:230:21: error strings should not be capitalized or end with punctuation or a newline

@silverwind silverwind changed the title Speed up golangci-lint Go linting adjustments Nov 26, 2020
@silverwind
Copy link
Member Author

silverwind commented Nov 26, 2020

Revive still goes into node_modules and randomly produces errors while at it, I have no idea why. I'm thinking we should maybe replace build/lint.go with a static binary download for it.

Also, even with the explicit paths, I don't really see much if any speed improvement which is odd.

@silverwind
Copy link
Member Author

Doesn't look like this is really going anywhere and there's no actual speed gain, so closing this. We should look into that revive issue with node_modules later.

@silverwind silverwind closed this Nov 27, 2020
@silverwind silverwind deleted the lintspeed branch November 27, 2020 00:22
@techknowlogick techknowlogick removed this from the 1.14.0 milestone Nov 27, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/code-linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants