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 golangci #6418

Open
wants to merge 51 commits into
base: master
from

Conversation

7 participants
@kolaente
Copy link
Member

commented Mar 23, 2019

This pr adds golangci to Gitea and replaces some linters (which golangci contains). This will (hopefully) enhance code quality.

Golangci lint calls a lot of golang lint tools directly, which make it a lot faster.

kolaente added some commits Mar 23, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2019

Looks like the new ci step works, now we just have to fix all the issues.

kolaente added some commits Mar 23, 2019

kolaente added some commits Mar 24, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Mar 24, 2019

Fixed most of the issues, however there are some issues where I don't really know what to do:

  • [ ]
modules/indexer/issues/indexer.go:95:32: Error return value of `issueIndexerUpdateQueue.Run` is not checked (errcheck)
        go issueIndexerUpdateQueue.Run()
                                      ^

It looks like issueIndexerUpdateQueue.Run() doesn't return an error in any of its implementations. Could we just remove the error return?

  • Fixed from the hint by @lunny.
models/git_diff.go:387:2: S1001: should use copy() instead of a loop (gosimple)
        for idx, lof := range hunk[:headerLines] {
        ^

I don't know how it is possible to let copy only copy some parts of a slice.

  • Done
models/repo_list.go:149:2: SA9004: only the first constant in this group has an explicit type (staticcheck)
        SearchOrderByAlphabetically        SearchOrderBy = "name ASC"
        ^

I don't really think this is an issue... or is it?

  • @jonasfranz said the check isn't needed at all, so I removed it completly.
routers/user/oauth.go:166:2: SA4006: this value of `errs` is never used (staticcheck)
        errs = form.Validate(ctx.Context, errs)
        ^

If the errors are not checked, can the whole check be removed? Or how can we check for the error?

Merge branch 'gitea-master' into golanci
# Conflicts:
#	models/repo_permission.go
@lunny

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@kolaente Is golangci support gitea? Since we will host gitea on gitea.com. If golangci don't support gitea, it will not work.

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

@lunny It's just a binary file which runs locally and in drone. Since we run it on our own infrastructure, we're not tied to a particular platform.

IIRC they offer tight ingration with github (things like having a bot which comments on the pr instead of just failing the pipeline) which we won't have. But I see no problem with it just failing the pipeline.

kolaente added some commits Mar 27, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2019

Just merged with Gitea's master, it did containt a lot of new issues which had to be fixed...

Show resolved Hide resolved .drone.yml Outdated

kolaente added some commits Mar 27, 2019

@sapk

sapk approved these changes Apr 3, 2019

Copy link
Member

left a comment

That a lot of missed error checking and more could still be logged in goroutine or defer. This will really improve the codebase and review.
LGTM, I would suggest to disable most of the linter that still failed and manage to enable them one at a time in separates PRs. This will ease the process of review and limit the number of conflict appearing.

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Apr 3, 2019

@sapk
Copy link
Member

left a comment

Found one template file to be removed

Show resolved Hide resolved routers/user/profile.go

kolaente added some commits Apr 6, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

@sapk I think we need to enable these now, I doubt there will be other prs soon otherwise...

kolaente added some commits Apr 17, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Bleve search tests are failing, not sure why.

kolaente added some commits Apr 17, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@lunny do you have an idea about the failing Bleve tests?

@@ -73,9 +73,9 @@ func PrintCurrentTest(t testing.TB, skip ...int) {
_, filename, line, _ := runtime.Caller(actualSkip)

if log.CanColorStdout {
fmt.Fprintf(os.Stdout, "=== %s (%s:%d)\n", log.NewColoredValue(t.Name()), strings.TrimPrefix(filename, prefix), line)
_, _ = fmt.Fprintf(os.Stdout, "=== %v (%s:%d)\n", log.NewColoredValue(t.Name()), strings.TrimPrefix(filename, prefix), line)

This comment has been minimized.

Copy link
@zeripath

zeripath Apr 18, 2019

Contributor

This should remain as %s not %v. Golang-ci is wrong here. ColoredValues have a format.

This comment has been minimized.

Copy link
@kolaente

kolaente Apr 18, 2019

Author Member

As disscussed in discord, I'm going to change the signature to return a fmt.Formatter instead.

@lunny

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

@kolaente It seems not only bleve. I found many failures on CI. https://drone.gitea.io/go-gitea/gitea/8110/1/9

kolaente added some commits Apr 22, 2019

@kolaente

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Show resolved Hide resolved integrations/git_test.go Outdated
Show resolved Hide resolved modules/lfs/locks.go Outdated
Show resolved Hide resolved modules/lfs/locks.go Outdated

zeripath and others added some commits Apr 22, 2019

Update modules/lfs/locks.go
Co-Authored-By: kolaente <konrad@kola-entertainments.de>
Update modules/lfs/locks.go
Co-Authored-By: kolaente <konrad@kola-entertainments.de>
Update modules/lfs/locks.go
Co-Authored-By: kolaente <konrad@kola-entertainments.de>
Show resolved Hide resolved modules/repofiles/update.go Outdated

kolaente added some commits Apr 22, 2019

fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.