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

chore: new make format, and run it #12867

Closed
wants to merge 34 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Aug 9, 2022

Description

  • changes make format
  • runs make format

Excludes all generated files and replaces gofmt and goimports with gofumpt


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)

@faddat faddat requested a review from a team as a code owner August 9, 2022 09:45
@faddat faddat changed the title new make format, and run it chore: new make format, and run it Aug 9, 2022
@faddat
Copy link
Contributor Author

faddat commented Aug 9, 2022

NB: the final change included replacing gofmt and goimports in .golangci.yml with gofumpt

the other two conflict with nolintlint ; gofumpt is just better anyhow and is now default in make format

@alexanderbez
Copy link
Contributor

@faddat can you look into the few CI failures pls?

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! See #12851 (comment) 🙈
One question, why do we revert the use of buildjet?

@julienrbrt julienrbrt mentioned this pull request Aug 9, 2022
19 tasks
@julienrbrt
Copy link
Member

julienrbrt commented Aug 10, 2022

@faddat can you look into the few CI failures pls?

The test 00 and test 01 fail are unrelated and due to the CI running Go 1.19.

TestCompose is a flaky test

@faddat
Copy link
Contributor Author

faddat commented Aug 10, 2022

@julienrbrt -- they have a new but determistic output.

Should I change the expected output in the test files?

Re: buildjet - that's a test so we can compare.

Also, seems like codeql really benefits from buildjet.

@faddat
Copy link
Contributor Author

faddat commented Aug 10, 2022

@julienrbrt hey I think that I went out of scope with the buildjet changes.

I am going to revert those and make them in another branch.

@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #12867 (17f103b) into main (461cfd6) will decrease coverage by 0.01%.
The diff coverage is 72.22%.

❗ Current head 17f103b differs from pull request most recent head 5e249e8. Consider uploading reports for the commit 5e249e8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12867      +/-   ##
==========================================
- Coverage   56.20%   56.18%   -0.02%     
==========================================
  Files         647      644       -3     
  Lines       55036    54899     -137     
==========================================
- Hits        30933    30847      -86     
+ Misses      21607    21562      -45     
+ Partials     2496     2490       -6     
Impacted Files Coverage Δ
baseapp/baseapp.go 76.87% <ø> (ø)
baseapp/msg_service_router.go 80.88% <ø> (ø)
client/keys/add.go 63.34% <ø> (ø)
codec/amino_codec.go 100.00% <ø> (ø)
codec/proto_codec.go 61.31% <ø> (ø)
crypto/hd/hdpath.go 98.40% <ø> (ø)
crypto/keys/multisig/codec.go 100.00% <ø> (ø)
crypto/keys/secp256k1/secp256k1.go 88.63% <ø> (ø)
crypto/keys/secp256k1/secp256k1_nocgo.go 83.33% <ø> (ø)
server/config/toml.go 66.66% <0.00%> (ø)
... and 86 more

@alexanderbez
Copy link
Contributor

--- FAIL: TestAppStateDeterminism (201.85s)
panic: group policies: unique constraint violation [recovered]
	panic: group policies: unique constraint violation

goroutine 13 [running]:
testing.tRunner.func1.2({0x1f33fe0, 0xc010f01800})
	/opt/hostedtoolcache/go/1.18.5/x64/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.18.5/x64/src/testing/testing.go:1392 +0x39f
panic({0x1f33fe0, 0xc010f01800})
	/opt/hostedtoolcache/go/1.18.5/x64/src/runtime/panic.go:838 +0x207
github.com/cosmos/cosmos-sdk/x/group/keeper.Keeper.InitGenesis({{0x2b35cb0, 0xc0301b18c0}, {0x7feb04dc7af8, 0xc030a20c60}, {0xc030a21620, {0x1}}, {0x2b4f280, 0xc030ab0440}, {0xc030a21680}, {0x2b4f280, ...}, ...}, ...)
	/home/ubuntu/actions-runner/_work/cosmos-sdk/cosmos-sdk/x/group/keeper/genesis.go:27 +0x665
github.com/cosmos/cosmos-sdk/x/group/module.AppModule.InitGenesis(...)
	/home/ubuntu/actions-runner/_work/cosmos-sdk/cosmos-sdk/x/group/module/module.go:122

@faddat
Copy link
Contributor Author

faddat commented Aug 10, 2022

@alexanderbez consistent?

@faddat
Copy link
Contributor Author

faddat commented Aug 10, 2022

I am going to decompose this further-- first Makefile, then changes....

@faddat
Copy link
Contributor Author

faddat commented Aug 10, 2022

closing to make smaller.

@faddat faddat closed this Aug 10, 2022
@julienrbrt julienrbrt mentioned this pull request Aug 10, 2022
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants