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

🌟 Let's talk about "formatters" #5296

Open
ldez opened this issue Jan 6, 2025 · 7 comments
Open

🌟 Let's talk about "formatters" #5296

ldez opened this issue Jan 6, 2025 · 7 comments
Assignees
Labels
area: config Related to .golangci.yml and/or cli options proposal
Milestone

Comments

@ldez
Copy link
Member

ldez commented Jan 6, 2025

Important

This is a proposal: I don't know if it is possible and what the impact could be inside the code.
The proposal may evolve.


Inside golangci-lint there are 4 "linters" that are not real linters/analyzers but "formatters": gofmt, goimports, gofumpt, and gci.

Their standard outputs are []byte and not Diagnostic, internally golangci-lint produces diff patches based on []byte and converts them into Diagnostic.

Also goimports and gci are not "import formatters": they format all the code and not only imports.

tool format imports order fix imports rewrite-rules simplify extra
gofmt x x (alphabetically) x x
goimports x x (multiple imports) x
gofumpt x x (one import) x x
gci x x (customizable)

⛑️ The Problems

Types

The formatters don't require types, but when they are run with other linters that require types, the possibilities of formaters to apply format and fix the code are limited.

Today, for example, you can use the following command to apply the format without being constraint by the other linters:

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

It works but this is limited.

💭 The Proposal

The idea is to create a dedicated section formatters like linters (internally they will be used as the other linters).

And adds a specific command fmt that runs only the formatters defined inside the configuration, like a variant of the run command but limited to formatters and with "autofix" always enabled, so no reports.

It allows us to clarify the difference between the formatters and the linters.

Also, we can set up a dedicated default for this section.
I suggest goimports as default because this is an official Go tool, more powerful than gofmt, and fits the needs of many projects.

formatters:
  enable:
    - gofumpt
    - gofmt
    - goimports
    - gci
  settings:
    gofumpt:
      opt1: true
    # ...

Enabling all the formatters make no sense as they provide the same base functionality (format) with some variants but their changes could be incompatible (mainly on imports) unless they are run in a specific order and without incompatible settings. So no need for enable-all and/or a disable section.

@ldez ldez added area: config Related to .golangci.yml and/or cli options proposal labels Jan 6, 2025
@ldez ldez added this to the v2 milestone Jan 6, 2025
@StevenACoffman
Copy link
Contributor

This is great! We like to run gofumpt + gci prior to other linters, since these are reliable and the suggested fixes of other linters require greater scrutiny.

@ldez
Copy link
Member Author

ldez commented Jan 8, 2025

My idea is to create a fmt command as a first step.

This command should work as gofmt, goimports, and the others: the formatting is not limited to the module files but to all the Go-files (workspaces and submodules).

This is completely different from the run command.

Note, this command will not have the same flag option as the gofmt, goimports, etc. because it doesn't fit the philosophy of golangci-lint: straight to the goal to be easy to use.
This means:

  • no -w, -d, etc. flags
  • no need to add . as the first arg (no stdin)
  • the files are only "fixed"/formatted

There is no major problem with browsing and formatting all the files (I have a working POC).

But there is a need for exclusions and those exclusions should be "compatible" with the current exclusion system as the formatters are also run as linters.
We cannot use the current exclusion processors (exclude-files,exclude-dirs, excludes, etc.) because this is not the same process (as explained before) and the exclusions don't have the same meaning for a formatter and a linter.

So I will need to create a dedicated section formatters.exclusions.

This implies:

  • to have the formatters section
  • to have something to exclude paths (no need for excludes, exclude-rules)
  • this option to exclude paths should be compatible (convertible) to something usable by the run command.

I don't want to introduce an option that we will deprecate (i.e. exclude-files,exclude-dirs), see #5299.

This requires creating the new option formatters.exclusions[].paths.
By creating this new option, I need to be able to fix #1178 and #3717.

The conversion will not be formatters.exclusions[].paths to issues.exclude-path (or issues.exclusions[].paths)
but formatters.exclusions[].paths to issues.exclude-rules (or linters.exclusions[].rules).

Also output.path-prefix could be a problem, I need to check that.


You may think that creating the formatters section (with just enable and settings) before the command can be a better first step, you may be right I need to think about that 🤔

I also need to check what is the right name formatters or formaters.

@StevenACoffman
Copy link
Contributor

StevenACoffman commented Jan 8, 2025

Very excited!

formatters over formaters please. 😆

After that this in place, there are a few other formatters that might be added that were never in golangci-lint before because they don't act as linters. For instance, https://github.com/segmentio/golines

also, gofumpt currently has an environment variable GOFUMPT_SPLIT_LONG_LINES="on" to control some behavior but it is not well documented mvdan/gofumpt#2

@ldez
Copy link
Member Author

ldez commented Jan 8, 2025

FYI I created a local implementation for golines.

But I will open a PR after the work on this topic.

@ldez ldez self-assigned this Jan 8, 2025
@ldez
Copy link
Member Author

ldez commented Jan 8, 2025

I already have 3 branches for this topic (command, exclude-path, golines), and after more thinking, I need to handle the exclusions section before everything 😢.
So I switch to #5297 for now.

@StevenACoffman
Copy link
Contributor

Ok, btw, not sure if plugins will be able to signal that they are formatters so they could be included in this or if that's something separate.

@ldez
Copy link
Member Author

ldez commented Jan 8, 2025

We should keep things "simple", so no formatters plugins for now, there are enough challenges with the current roadmap of the v2.

Maybe, if we are able to have an MRR over 5000€, we will add some items to the roadmap 😸

@StevenACoffman Thank you for your engagement and the donation, I hope that more people will follow this way.

I will wait until the end of the month, but if the situation doesn't change, I will have to be more pushy with the sponsorship.

For golangci-lint users, if you read this comment:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: config Related to .golangci.yml and/or cli options proposal
Projects
None yet
Development

No branches or pull requests

2 participants