chore: gofumpt

[faddat]

Description

I generated this pr using gofumpt

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

I find that it really does a great job of keeping things clean and
readable. Is it something we want to use?

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[tac0turtle]

Love the clean up, went through parts of the PR not everything due to size. Would love a second approval before merging.

[julienrbrt]

Can this be as well added in .golangci.yml?

[alexanderbez]

Didn't we have issues with this on Osmosis? What does this buy us over goimports which is natively supported by gopls and vscode?

[faddat]

@alexanderbez yep, we did.

I figure if we do it once in a while, we can get the benefits without the pain of having it in ci.

In ci, it could cause pain because then everyone must

gofumpt -w -l . every time

@ValarDragon mentioned it could be rough for first timers and I agree.

Also, I think it makes the code look and feel nicer.

Maybe do once in a while for a bit, and when the tooling/practice is more mature we add it to ci.

For example if it ran automatically when users:

golangci-lint run --fix ./... that'd be awesome

[amaury1093]

Perhaps we can add gofumpt'ing the entire code to a script that we execute pre-releases and prior to making new major release branches?

I also think this is the best idea for now (suggestion from the osmosis issue)

[tac0turtle]

@faddat do you want to add this in the release process?

[faddat]

@marbar3778 Sure!

[faddat]

@marbar3778 I've added gofumpt to the release process description document.

[julienrbrt]

All changes checked. Approving as lgtm.

However, just a few nits. I think gofumpt made some formatting that is correct but not optimal due to the current formatting: when we were already line wrapping the arguments, gofumpt sometimes gets us one line with only one argument. IMHO we should then use only one line, or remove our manual wrapping and let gofumpt do its thing.

I think it'd be great to fix them before merging, as we are touching all those files anyway.

[faddat]

@julienrbrt -- I cannot exactly explain / justify changes maade by gofumpt -w -l .

Overall, I like what it does for the appearance and standardization of code, and you can check it out in more detail here:

https://github.com/mvdan/gofumpt

Sorry bout that

[julienrbrt]

@julienrbrt -- I cannot exactly explain / justify changes maade by gofumpt -w -l .

Overall, I like what it does for the appearance and standardization of code, and you can check it out in more detail here:

https://github.com/mvdan/gofumpt

Sorry bout that

I understand, I use the tool as part of my workflow as well.
What I meant is that you can tweak how gofumpt will format lines by improving the formatting yourself first. My comments were suggestions where to do that:

For instance, in x/distribution/types/query.go we have:

func NewQueryDelegatorTotalRewardsResponse(rewards []DelegationDelegatorReward,
	total sdk.DecCoins) QueryDelegatorTotalRewardsResponse {
	return QueryDelegatorTotalRewardsResponse{Rewards: rewards, Total: total}
}

running gofumpt -w -l . on that will give us

func NewQueryDelegatorTotalRewardsResponse(rewards []DelegationDelegatorReward,
	total sdk.DecCoins,
) QueryDelegatorTotalRewardsResponse {
	return QueryDelegatorTotalRewardsResponse{Rewards: rewards, Total: total}
}

which is weird.

If before running gofumpt, we improve the styling to

func NewQueryDelegatorTotalRewardsResponse(rewards []DelegationDelegatorReward, total sdk.DecCoins) QueryDelegatorTotalRewardsResponse {
	return QueryDelegatorTotalRewardsResponse{Rewards: rewards, Total: total}
}

nothing will change, and we get, imo a better styling than with gofumpt.

I think it worth it to do that manual improving where we see gofumpt format the code with a one argument line.

[alexanderbez]

I think I agree @julienrbrt. I should either be one line or each arg in a separate line. What gofumpt produces,

func NewQueryDelegatorTotalRewardsResponse(rewards []DelegationDelegatorReward,
	total sdk.DecCoins,
) QueryDelegatorTotalRewardsResponse {
	return QueryDelegatorTotalRewardsResponse{Rewards: rewards, Total: total}
}

Is not great. I'd rather see:

func NewQueryDelegatorTotalRewardsResponse(
	rewards []DelegationDelegatorReward,
	total sdk.DecCoins,
) QueryDelegatorTotalRewardsResponse {
	return QueryDelegatorTotalRewardsResponse{Rewards: rewards, Total: total}
}

[julienrbrt]

@faddat Do you mind If I fix these nits myself in your branch?

[faddat]

I don't mind at all :).

I am also wondering how else we can improve. Your nits were real.

aha!

OK, now that I have re-read your comments, it makes sense.

1st update:
I think I can find time tomorrow, or can give you access to our repo. Thanks a bunch for the review and suggestions!

2nd update:
thanks for the pr!

[julienrbrt]

Thanks for updating!