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

🧹 Linting #1418

Merged
merged 120 commits into from
Jul 28, 2022
Merged

🧹 Linting #1418

merged 120 commits into from
Jul 28, 2022

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 23, 2022

Description

This PR updates the linters so that we no longer use deprecated linters, enforces linting in CI, and fixes linter errors.

closes: #1417

Please note that this changes a number of important names, but likely most important:

every time we named something with Id, that has been changed to ID per revive.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #1418 (db496c2) into main (8987888) will decrease coverage by 0.07%.
The diff coverage is 76.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1418      +/-   ##
==========================================
- Coverage   80.05%   79.97%   -0.08%     
==========================================
  Files         164      165       +1     
  Lines       12300    12367      +67     
==========================================
+ Hits         9847     9891      +44     
- Misses       1993     2012      +19     
- Partials      460      464       +4     
Impacted Files Coverage Δ
modules/apps/27-interchain-accounts/module.go 54.87% <0.00%> (-2.82%) ⬇️
modules/apps/29-fee/module.go 53.57% <0.00%> (-1.99%) ⬇️
modules/apps/transfer/keeper/relay.go 88.11% <ø> (ø)
modules/core/02-client/keeper/client.go 98.22% <ø> (ø)
modules/core/02-client/keeper/grpc_query.go 68.20% <ø> (ø)
modules/core/02-client/keeper/proposal.go 86.36% <ø> (ø)
modules/core/03-connection/keeper/handshake.go 89.83% <ø> (-0.06%) ⬇️
modules/core/03-connection/keeper/keeper.go 90.43% <ø> (ø)
modules/core/04-channel/types/packet.go 67.56% <0.00%> (ø)
modules/core/keeper/msg_server.go 58.29% <ø> (ø)
... and 28 more

@faddat
Copy link
Contributor Author

faddat commented May 23, 2022

The first run failed due to timeout, so I gave it a longer timeout.

@faddat faddat requested a review from chatton as a code owner May 26, 2022 06:23
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thanks for these improvements, @faddat!

I think it would also make sense to add a brief explanation of the API breaking changes in the migration docs. If you prefer, we can take care of that ourselves in a separate PR.

CHANGELOG.md Outdated Show resolved Hide resolved
modules/core/ante/ante.go Outdated Show resolved Hide resolved
Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Thanks Jacob, I'll defer to the implementation team here. I just have a couple of questions/suggestions

modules/core/02-client/legacy/v100/store.go Outdated Show resolved Hide resolved
modules/core/04-channel/types/events.go Show resolved Hide resolved
testing/app.go Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

Note: some of these defy normal convention in Cosmos. What I would like to do is come up with a standard set of linters that we can use for SDK 46 onwards, so that we end up not triggering liter errors and needing to use golint. I started work on that over at Osmosis, and eventually hope to apply that work to the Cosmos SDK.

.golangci.yml Show resolved Hide resolved
modules/apps/29-fee/ibc_middleware.go Outdated Show resolved Hide resolved
modules/apps/transfer/simulation/params.go Show resolved Hide resolved
modules/core/04-channel/keeper/events.go Outdated Show resolved Hide resolved
modules/core/04-channel/keeper/events.go Show resolved Hide resolved
modules/core/ante/ante.go Outdated Show resolved Hide resolved
modules/core/ante/ante.go Outdated Show resolved Hide resolved
modules/core/ante/ante.go Outdated Show resolved Hide resolved
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
@faddat
Copy link
Contributor Author

faddat commented Jul 26, 2022

I suspect they'll perpetually be grouped with stdlib, but I'll kick that off now.

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM, only minor concerns left:

  • e2e module naming change (think this is likely fine as is)
  • RedundancyDecorator renamed to RedundantRelayDecorator? Did we want to do this change?

e2e/go.mod Show resolved Hide resolved
@damiannolan
Copy link
Member

damiannolan commented Jul 27, 2022

LGTM, only minor concerns left:

  • e2e module naming change (think this is likely fine as is)
  • RedundancyDecorator renamed to RedundantRelayDecorator? Did we want to do this change?

Do you think we could additionally add an exception to ignore .pb.go files for goimports ordering? Generally these should be read-only afaik! cc. @faddat

Thanks for all your work on getting this over the line!

@faddat
Copy link
Contributor Author

faddat commented Jul 27, 2022

@vuong177 is re-doing protos now

make proto-all doesn't work on my computer for some reason

@faddat
Copy link
Contributor Author

faddat commented Jul 27, 2022

RedundancyDecorator renamed to RedundantRelayDecorator? Did we want to do this change?

-- I don't think so?

I'll have a look and try to figure out how it got in because I don't recall making it. maybe it triggered the "stutters" thing? checking...

@colin-axner
Copy link
Contributor

RedundancyDecorator renamed to RedundantRelayDecorator? Did we want to do this change?

That seemed to be the conclusion to me, but I don't have a strong preference. It just seems like a more descriptive name. Did it cause an issue with the linter?

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Great work!! Thanks for following up on all our suggestions. I'm still curious about the Decorator naming, but don't want to block merge on that discussion

@crodriguezvega
Copy link
Contributor

@faddat There seem to be some conflicts...

@crodriguezvega crodriguezvega added this to the v5.0.0 milestone Jul 27, 2022
@faddat
Copy link
Contributor Author

faddat commented Jul 28, 2022

@colin-axner let's keep the name as it is, then. It doesn't cause an issue with the linter. In fact, once this is merged, our editors should be able to scream at us about even the most trivial lint issues. It's 100% golang-ci lint'd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

linting
9 participants