Skip to content

chore: gofumpt#8478

Closed
faddat wants to merge 1 commit intotendermint:masterfrom
faddat:faddat/gofumpt
Closed

chore: gofumpt#8478
faddat wants to merge 1 commit intotendermint:masterfrom
faddat:faddat/gofumpt

Conversation

@faddat
Copy link
Contributor

@faddat faddat commented May 6, 2022

This PR runs gofumpt.

It can be replicated:

go install mvdan.cc/gofumpt@latest
gofumpt -l -w .

It could be enabled in .golangci.yml but that may not be desiralbe as it
adds another step for those contributing to tendermint.

I've found this approach to be extremely helpful because of the rigor and
regularity that gofumpt adds to code.

Here is a related PR on cosmos-sdk:

cosmos/cosmos-sdk#11839

@creachadair
Copy link

This change is mechanically fine, but I'm not enthusiastic about the gofumpt tool.

Most of what it does is individually harmless, but it generates a lot of essentially-vacuous diffs in the Git history and doesn't provide all that much benefit (apart from a few mild cosmetics). As someone who relies a lot on tracking git blame to find when stuff changed, I prefer not to churn the code with whitespace changes.

Also, as a matter of ongoing maintenance, we would either have to enforce this style in CI (thus forcing everyone else to install this tool too), or it will bitrot.

Others may feel differently, but for those reasons I would rather not do this globally. If we do want this, I think it's better to clean up each package individually when we're doing other work on it anyway.

That said—others on the team may feel differently.

@williambanfield
Copy link
Contributor

That said—others on the team may feel differently.

I feel similarly, the git log / git blame is hugely important to me. Making many changes for small cosmetic updates is of little utility in my view.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale for use by stalebot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants