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

feat: new fmt command with dedicated formatter configuration #5357

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ldez
Copy link
Member

@ldez ldez commented Jan 28, 2025

Important

This PR deliberately omits documentation about the new options and section (but the JSONSchema is updated).
This is because I don't want to deprecate the previous elements for now: I think it is better to handle that when all the v2 proposals are managed to avoid multiple migrations.


The command golangci-lint fmt formats the code by default.

The current default is the same format used by go fmt (!= gofmt).

There is no support for stdin.

The formatters defined in the formatters.enable are added automatically to linters.enable.

If a formatter is enabled inside formatters and linters, the configuration of formatters.settings will override linters-settings even if the configuration is omitted or empty inside formatters.settings section.
This means the formatter activation and configurations should be defined only on the formatters or linters+linters-settings section and not mixed.

The exclusions from formatters.exclusions[].paths are converted to linters.exclusions[].rules automatically.

There are only 3(4) flags:

Flags:
  -c, --config PATH      Read config from file path PATH
      --no-config        Don't read config file
  -E, --enable strings   Enable specific formatter
  -d, --diff             display diffs instead of rewriting files

Note, that the -E, --enable flags work like --enable-only for linters: it overrides the formatters defined inside the configuration file.
It is the opposite of the behavior of the -E, --enable flags of the run command where this flag always adds linters to those defined inside the configuration file.

In v2, the configuration of formatters will not be allowed inside the linters-settings section.

formatters:
  enable:
    - gofumpt
    - gofmt
    - goimports
    - gci
  settings:
    gofumpt:
      # ...
    gofmt:
      # ...
    goimports:
      # ...
    gci:
      # ...
  exclusions:
    generated: strict
    paths:
      - foo
      - bar
      # ...

Fixes #5296

@ldez ldez added enhancement New feature or improvement area: fmt labels Jan 28, 2025
@ldez ldez added this to the v2 milestone Jan 28, 2025
@ldez ldez requested review from bombsimon and alexandear January 28, 2025 22:53
@ldez ldez modified the milestones: v2, next Jan 28, 2025
pkg/config/formatters_settings.go Show resolved Hide resolved
pkg/config/loader.go Show resolved Hide resolved
pkg/config/loader.go Show resolved Hide resolved
cmd/golangci-lint/main.go Show resolved Hide resolved
pkg/commands/fmt.go Outdated Show resolved Hide resolved
pkg/goformat/runner.go Show resolved Hide resolved
pkg/goformat/runner.go Outdated Show resolved Hide resolved
pkg/goformat/runner.go Outdated Show resolved Hide resolved
pkg/goformat/runner.go Outdated Show resolved Hide resolved
pkg/goformat/runner.go Show resolved Hide resolved
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

LGTM!

I think making this still supported as a linter and available with the --fix flag is a great suggestion.

Have you done any testing on performance? I'd imagine golangci-lint fmt on big codebases would potentially be faster than golangci-lint run --fix or are they the same? When would you suggest one over the other? On save in your editor?

pkg/config/formatters_settings.go Show resolved Hide resolved
pkg/config/loader.go Show resolved Hide resolved
pkg/goformat/runner.go Show resolved Hide resolved
@ldez
Copy link
Member Author

ldez commented Feb 1, 2025

Have you done any testing on performance? I'd imagine golangci-lint fmt on big codebases would potentially be faster than golangci-lint run --fix or are they the same?

Yes, I did it.

The fmt is significantly faster than run.
Because it's only focused on pure syntax analyzing, no type loading, no cache filling, etc.

When would you suggest one over the other? On save in your editor?

The 2 commands have different goals, I cannot suggest one over the other.

But I suggest always running fmt first because it's quick and safe.

@ldez ldez removed this from the next milestone Feb 1, 2025
@ldez
Copy link
Member Author

ldez commented Feb 1, 2025

I will not merge this PR for now: based on Go release cycle, go1.24 will be released in 2 weeks.

I want to keep this feature for v2 and I want the last v1 release to support go1.24.

Also, for now, I don't want to play with a dedicated v2 branch.

This is frustrating for me because I want to see this new command used and I have another PR ready based on the branch of this PR.

@ldez ldez added this to the v2 milestone Feb 1, 2025
@ldez ldez force-pushed the feat/fmt-cmd branch 2 times, most recently from 3622326 to 1d8c9c1 Compare February 3, 2025 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: fmt enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🌟 Let's talk about "formatters"
3 participants