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 lint.sh to run golanci-lint in 2 stages #2051

Merged
merged 3 commits into from
Dec 30, 2024
Merged

Add lint.sh to run golanci-lint in 2 stages #2051

merged 3 commits into from
Dec 30, 2024

Conversation

denik
Copy link
Contributor

@denik denik commented Dec 30, 2024

First stage is to run goimports and formatter, second is full suite.

This ensures that imports and formatting are fixed even in presence of other issues. Otherwise golanci-lint refuses to fix anything golangci/golangci-lint#5257

This helpful when running aider with config like this - aider will use that to autofix what it can after every update:

% cat .aider.conf.yml
lint-cmd:
  - "go: ./lint.sh"

First stage is to run goimports and formatter, second is full suite.

This ensures that imports and formatting are fixed even in presence of
other issues. Otherwise golanci-lint refuses to fix anything.

This helpful when running aider with config like this - aider will use that
to autofix what it can after every update:

% cat .aider.conf.yml
lint-cmd:
  - "go: ./lint.sh"
@denik denik temporarily deployed to test-trigger-is December 30, 2024 13:18 — with GitHub Actions Inactive
Copy link

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/cli

Inputs:

  • PR number: 2051
  • Commit SHA: b9e88d51953b2f801d72cd5a2144388af0198054

Checks will be approved automatically on success.

@denik denik temporarily deployed to test-trigger-is December 30, 2024 13:19 — with GitHub Actions Inactive
# Fixing formatting is also reasonable thing to do.
# For this reason, this script runs golangci-lint in two stages:
golangci-lint run --fix --no-config --disable-all --enable gofumpt,goimports $@
exec golangci-lint run --fix $@
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work on CI? We don't actually commit anything back, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither lint.sh not "make lint" are called on CI. It uses golangci's github action instead: https://github.com/databricks/cli/blob/main/.github/workflows/push.yml#L75

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we're not even going to modify anything in the worktree, because we don't pass --fix there. If there are any mismatches, they won't be corrected, the check will fail.

@denik denik requested a review from andrewnester December 30, 2024 15:18
@denik denik added this pull request to the merge queue Dec 30, 2024
Merged via the queue into main with commit e088d0d Dec 30, 2024
9 checks passed
@denik denik deleted the denik/lint.sh branch December 30, 2024 15:25
# However, running goimports first alone will actually fix some of the compilation issues.
# Fixing formatting is also reasonable thing to do.
# For this reason, this script runs golangci-lint in two stages:
golangci-lint run --fix --no-config --disable-all --enable gofumpt,goimports $@
Copy link

@ldez ldez Dec 30, 2024

Choose a reason for hiding this comment

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

@denik I would recommend this:

golangci-lint run --enable-only="gofmt,gofumpt,goimports" --fix

Because this will read the configuration of the formatters but ignore the rest.

Those 3 linters just read the syntax, not the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Yes, that makes sense. #2056

denik added a commit that referenced this pull request Dec 30, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 30, 2024
denik added a commit that referenced this pull request Jan 2, 2025
Follow up to #2051 and #2056.

Running golangci-lint twice always is measurably slower (1.5s vs 2.5s).
denik added a commit that referenced this pull request Jan 2, 2025
Follow up to #2051 and #2056.

Running golangci-lint twice always is measurably slower (1.5s vs 2.5s),
so only run it twice in case it is necessary.
denik added a commit that referenced this pull request Jan 10, 2025
See Makefile for explanation on difference between 'make fmt' and 'make
lint'.

I also removed lint.sh. Original motivation was to use it in aider, but
it's not a good fit there, because aider passes filenames and it does
not work well with most golang linters which requires whole packages to
work.

Follow up to #2062, #2056, #2051.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants