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

ci: use thelper #3809

Merged
merged 13 commits into from
Jun 15, 2023
Merged

ci: use thelper #3809

merged 13 commits into from
Jun 15, 2023

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 10, 2023

This PR removes the depguard linter and enables thelper. It will make upgrading
to v50 easier.

  • tidy and enable thelper
  • use thelper and disable depguard
  • use latest golangci-lint in ci

@crodriguezvega
Copy link
Contributor

Hey @faddat, can you please give us a bit more of context?

  • What's the reason to remove depguard?
  • What benefits does thelper bring?
  • Why would these changes make upgrading to SDK v0.50 easier?

Thanks!

@faddat
Copy link
Contributor Author

faddat commented Jun 12, 2023

Sure I do very happy to tell you a bit more about this one, so the thelper linter really just reminds devs to use the call t.Helper()

And when you do that, your IDE can give you some additional information for debugging. I would like to use it during the upgrade to SDK 50 because that useful information just makes the job easier.

depguard .... I got to tell you I think it never worked. But you see them the thing is it started working and when it began to work it started doing some kind of bad stuff, like blocking every single cosmos import, so I pulled it out.

Anyhow this should make every upgrade easier, not just SDK 50

@DimitrisJim
Copy link
Contributor

What's the reason to remove depguard?

Looking into this can't see why we already have depguard, there's no configuration for it here or in the sdk. Seems like it can safely be removed.

@faddat
Copy link
Contributor Author

faddat commented Jun 12, 2023

Ah, yeah depguard totally needs to go imo :).

I think that it never worked before, now suddenly does, and is... not the tool for the job we are trying to do.

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM thanks @faddat

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm!

@codecov-commenter
Copy link

Codecov Report

Merging #3809 (b8e232f) into main (7ccabcb) will increase coverage by 0.01%.
The diff coverage is 75.78%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3809      +/-   ##
==========================================
+ Coverage   78.75%   78.76%   +0.01%     
==========================================
  Files         187      187              
  Lines       12890    12935      +45     
==========================================
+ Hits        10151    10188      +37     
- Misses       2308     2317       +9     
+ Partials      431      430       -1     
Impacted Files Coverage Δ
.../27-interchain-accounts/controller/types/params.go 100.00% <ø> (+45.00%) ⬆️
...erchain-accounts/controller/types/params_legacy.go 0.00% <0.00%> (ø)
...ps/27-interchain-accounts/genesis/types/genesis.go 0.00% <0.00%> (ø)
modules/apps/27-interchain-accounts/module.go 51.00% <22.22%> (-2.69%) ⬇️
...s/27-interchain-accounts/host/keeper/migrations.go 78.57% <57.14%> (ø)
...7-interchain-accounts/controller/ibc_middleware.go 70.96% <100.00%> (ø)
...27-interchain-accounts/controller/keeper/keeper.go 90.62% <100.00%> (+0.96%) ⬆️
...nterchain-accounts/controller/keeper/migrations.go 87.09% <100.00%> (+6.14%) ⬆️
...nterchain-accounts/controller/keeper/msg_server.go 93.47% <100.00%> (+1.37%) ⬆️
...s/27-interchain-accounts/controller/types/codec.go 100.00% <100.00%> (ø)
... and 1 more

@crodriguezvega crodriguezvega merged commit 8ccc873 into cosmos:main Jun 15, 2023
@faddat faddat mentioned this pull request Jun 18, 2023
9 tasks
@chatton chatton mentioned this pull request Jul 4, 2023
9 tasks
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.

5 participants