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

Enable more go linters [ready to split] #2960

Closed
wants to merge 25 commits into from
Closed

Conversation

xoxys
Copy link
Member

@xoxys xoxys commented Dec 17, 2023

Supersedes: #977

@xoxys xoxys marked this pull request as draft December 17, 2023 22:27
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Dec 17, 2023

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-2960.surge.sh

@xoxys
Copy link
Member Author

xoxys commented Dec 17, 2023

I'm a bit confused about how pre-commit is handling golangci-lint. While make lint still returns a lot of errors (PR not ready yet), pre-commit seems to be happy. Any idea @pat-s?

@xoxys xoxys added the refactor delete or replace old code label Dec 18, 2023
@xoxys
Copy link
Member Author

xoxys commented Dec 18, 2023

Some remaining issues where I would like to get some input:

  1. Why is != 2 correct here? Would like to document what 2 means and why it is the intended value.
server/forge/github/github.go:493:22: mnd: Magic number: 2, in <condition> detected (gomnd)
		if len(matches) != 2 {
		                   ^
  1. Why 8 bytes? Would like to document why it is the intended value.
pipeline/multipart/reader.go:55:21: mnd: Magic number: 8, in <argument> detected (gomnd)
	out, _ := buf.Peek(8)
	                   ^
  1. Why is < 2 correct here? Would like to document what 2 means and why it is the intended value.
pipeline/backend/docker/convert.go:125:19: mnd: Magic number: 2, in <condition> detected (gomnd)
		if len(parts) < 2 {
		                ^
pipeline/backend/docker/convert.go:152:19: mnd: Magic number: 2, in <condition> detected (gomnd)
		if len(parts) < 2 {
		                ^
  1. If the goal is to get seconds from the given duration, would it be better to use time.Duration(cf.timeout.Seconds())?
server/forge/configFetcher.go:64:30: Multiplication of durations: `time.Second * cf.timeout` (durationcheck)
		files, err = cf.fetch(ctx, time.Second*cf.timeout, strings.TrimSpace(cf.configPath))
		                           ^
  1. Why is <= 3 correct here? Would like to document what 3 means and why it is the intended value.
pipeline/shared/replace_secrets.go:23:18: mnd: Magic number: 3, in <condition> detected (gomnd)
		if len(old) <= 3 {
		               ^
  1. Is it acceptable to not exec the defer function in this case, or do we need to refactor this part?
cmd/server/server.go:154:4: exitAfterDefer: log.Fatal will exit, and `defer func(){...}(...)` will not run (gocritic)
			log.Fatal().Err(err).Msg("failed to create web engine")

@xoxys xoxys requested a review from a team December 18, 2023 10:19
shared/httputil/httputil.go Outdated Show resolved Hide resolved
cli/internal/util.go Outdated Show resolved Hide resolved
agent/runner.go Outdated Show resolved Hide resolved
pipeline/stepBuilder.go Outdated Show resolved Hide resolved
pipeline/tracer.go Outdated Show resolved Hide resolved
pipeline/backend/backend.go Outdated Show resolved Hide resolved
@xoxys
Copy link
Member Author

xoxys commented Dec 19, 2023

Thanks! Will take care.

pipeline/stepBuilder.go Outdated Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Jan 5, 2024

in that case I'll reopen so it dont get deleted and the pull remains as TODO

@qwerty287
Copy link
Contributor

qwerty287 commented Jan 5, 2024

Github never deletes code (except on force pushes) I think. The commits and PR diff should always be available, also on PR close/branch deletion.

@qwerty287 qwerty287 modified the milestones: 2.2.0, 3.x.x Jan 7, 2024
@qwerty287 qwerty287 mentioned this pull request Jan 7, 2024
@qwerty287 qwerty287 changed the title WIP: Enable more go linters [ready to split] Enable more go linters [ready to split] Jan 7, 2024
@qwerty287 qwerty287 removed this from the 3.x.x milestone Jan 7, 2024
6543 pushed a commit that referenced this pull request Jan 9, 2024
Mostly those that did not require much work.

From #2960

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@xoxys
Copy link
Member Author

xoxys commented Jan 11, 2024

I've added dedicated PRs for all missing linters. Closing again.

@xoxys xoxys closed this Jan 11, 2024
@6543 6543 deleted the linter_explosion_v2 branch January 11, 2024 17:05
lafriks pushed a commit that referenced this pull request Jan 11, 2024
6543 pushed a commit that referenced this pull request Jan 12, 2024
fernandrone pushed a commit to quintoandar/woodpecker that referenced this pull request Feb 1, 2024
Mostly those that did not require much work.

From woodpecker-ci#2960

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
fernandrone pushed a commit to quintoandar/woodpecker that referenced this pull request Feb 1, 2024
fernandrone pushed a commit to quintoandar/woodpecker that referenced this pull request Feb 1, 2024
fernandrone pushed a commit to quintoandar/woodpecker that referenced this pull request Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor delete or replace old code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants