Thanks for opening this PR Jacob.

I'm very hesitant of the large bulk of changes here. The reason for hesitation (like we discussed in the community calls regarding gofumpt in the old repo tendermint/tendermint#9738) is that Tendermint Core tags v0.35 and v0.36 still have valuable code waiting to be cherry picked into CometBFT. A concrete example is PBTS, which we plan to ship into v1 (#1731). Another good example is #1769.

If we allow linting or significant syntactic changes to bleed into CometBFT that will make porting code from the old repo significantly more difficult. Within several months from now we expect to not have this constraint anymore. But at the moment linting is a not a priority. PBTS is a priority. Hope this makes sense!

The comment from Sergio still applies: If you want to help, the packages listed there are safe to lint, but not wide-sweeping changes like in this PR.

The other open PRs seem to touch only tests, and that's better, but I'll have a look separately. Will leave this PR open until I double check more thoroughly.

@adizere - gofumpt has been applied to the whole master branch by @melekes -- just not the tests.

Additionally, gci has been applied to the whole master branch -- just not the tests, and it wasn't configured. When it's configured it is pretty great though!

That's what

enable-all does

Here is the PR that applied gofumpt and gci:

• golangci-lint: enable all the linters by default #1838

Seeing that PR made me so happy. I try not to work on code that hasn't been linted first, it gives your editor superpowers if the code is rigorously linted.

Thing is, it wasn't applied to the tests, leaving things inconsistent.

Why lint?

Well, have a look at the changes to the test code.

• We used assert.NotNil when we should be using require.Error.
• We used assert.Nil when we should have been using require.Error.
• We had unchecked errors.
• .....lots, lots more

The test code is highly inconsistent, and these inconsistencies are how we end up in a place of "let's just run the tests a few hundred more times to see which ones explode." (this is very literally where we are from what I can tell)

That is not where we want to be, so, we lint.

Please note that gci is also enabled by enable-all, and I fully agree with "Ideally we'd pass them all". It's how we can use tooling to improve reliability and performance.

And as a user of comet, I want the CI system and tests to function as a source of truth. I'm contributing the code that'll get us there.

• refactor node_test.go #1952 (comment)

Furthermore -- check this one out.

It does nothing but run the tests 40 times.

• run tests 40x against the unmodified master branch #1950

They fail intermittently, and this costs everyone -- both those working directly on comet, and downstream users -- time.

note

@adizere I just re-read your comments, and if you meant the re-organization of the imports caused by this PR then.... yeah actually that can cause conflicts, and your comments, in that light, may make a heck of a lot of sense.

Please feel free to close this, if that is what you meant.

If anything, the changes that you'd like to merge in @adizere -- to ensure that they're sound -- should be linted as well.

Gonna close this for now, @adizere -- please re-open if I misunderstood you.

Took me 3 reads but I get where you're coming from now.

Thanks, appreciate the flexibility! Will sync with team to understand what's best.