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

gofumpt to reduce total number of changes versus v5 #1734

Merged
merged 15 commits into from
Jul 27, 2022

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jul 19, 2022

Description

This is just a formatting change that will ease the review of v5.

This PR replaces #1717


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

dependabot bot and others added 3 commits February 10, 2022 03:04
Bumps [actions/setup-go](https://github.com/actions/setup-go) from 2.1.5 to 2.2.0.
- [Release notes](https://github.com/actions/setup-go/releases)
- [Commits](actions/setup-go@v2.1.5...v2.2.0)

---
updated-dependencies:
- dependency-name: actions/setup-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
…ctions/setup-go-2.2.0

build(deps): bump actions/setup-go from 2.1.5 to 2.2.0
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.

Looks good to me! But just out of curiosity: what was wrong with the previous PR that you needed to open this new one?

modules/apps/29-fee/ibc_middleware.go Outdated Show resolved Hide resolved
@faddat
Copy link
Contributor Author

faddat commented Jul 21, 2022

done :)

@crodriguezvega crodriguezvega mentioned this pull request Jul 21, 2022
9 tasks
@faddat
Copy link
Contributor Author

faddat commented Jul 22, 2022

Looks good to me! But just out of curiosity: what was wrong with #1717 that you needed to open this new one?

It was accumulating cruft from merging main in.

At least, I think that's the reason.

@faddat
Copy link
Contributor Author

faddat commented Jul 22, 2022

I think it's set.

@crodriguezvega
Copy link
Contributor

Do we still need this PR after we merge #1418? Or can this be closed then?

@faddat
Copy link
Contributor Author

faddat commented Jul 22, 2022

Either way really. You could merge this to make a final review of v5 easier, or merge v5 and you’d still get this.

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2022

Codecov Report

Merging #1734 (1288fb6) into main (af4e651) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1288fb6 differs from pull request most recent head 0929260. Consider uploading reports for the commit 0929260 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
- Coverage   80.04%   80.04%   -0.01%     
==========================================
  Files         166      166              
  Lines       12421    12419       -2     
==========================================
- Hits         9943     9941       -2     
  Misses       2013     2013              
  Partials      465      465              
Impacted Files Coverage Δ
cmd/build_test_matrix/main.go 74.28% <ø> (ø)
modules/apps/transfer/keeper/migrations.go 93.10% <ø> (-0.23%) ⬇️
modules/core/02-client/keeper/grpc_query.go 68.20% <ø> (ø)
modules/core/03-connection/keeper/handshake.go 89.83% <ø> (-0.06%) ⬇️
modules/apps/29-fee/ibc_middleware.go 91.83% <100.00%> (ø)

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, just curious on why we should add gitpod?

.gitpod.yml Outdated
@@ -0,0 +1,9 @@
# This configuration file was automatically generated by Gitpod.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why this is useful? I'm not familiar with Gitpod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh no I am sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so 1 bad thing about gitpod:

It puts these in repos automatically.

So I guess not seeing them in unrelated PR's could be a good reason to do it, but that's not really a good reason. Let me get into an actual good reason, after deleting this file.

sorry about this!
@faddat faddat mentioned this pull request Jul 25, 2022
9 tasks
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@colin-axner colin-axner enabled auto-merge (squash) July 26, 2022 09:48
auto-merge was automatically disabled July 27, 2022 14:02

Head branch was pushed to by a user without write access

@colin-axner colin-axner dismissed crodriguezvega’s stale review July 27, 2022 14:15

concerns addressed

@colin-axner colin-axner enabled auto-merge (squash) July 27, 2022 14:15
@colin-axner colin-axner merged commit 48c456d into cosmos:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants