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(api)!: add missing/remove unnecessary gogoproto directive #3856

Merged
merged 13 commits into from
Jun 22, 2023

Conversation

crodriguezvega
Copy link
Contributor

@crodriguezvega crodriguezvega commented Jun 15, 2023

Description

Adds

option (gogoproto.goproto_getters) = false;

where it was missing. See https://github.com/cosmos/ibc-go/pull/3845/files#r1229579617

Removes

option (gogoproto.equal)           = false;

Since it is not needed. See #3858 (comment)

closes: #3871

Commit Message / Changelog Entry

chore(api)!: add missing/remove unnecessary gogoproto directive

see the guidelines for commit messages. (view raw markdown for examples)


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@crodriguezvega crodriguezvega marked this pull request as draft June 15, 2023 13:40
controllerPortID, msgChanOpenInitRes.GetChannelId(),
msgChanOpenTryRes.GetChannelId(), msgChanOpenTryRes.GetVersion(),
controllerPortID, msgChanOpenInitRes.ChannelId,
msgChanOpenTryRes.ChannelId, msgChanOpenTryResVersion,
Copy link
Contributor

Choose a reason for hiding this comment

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

to save you the trouble of looking, msgChanOpenTry.Version, similarly in lines 244 and 374.

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, man, thank you so much. I was scratching my head what the error was. 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, the script that builds the e2e's discards stdout/stderr which was silly of me to add. Amending them now so errors like these won't be cryptic from now on 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

@crodriguezvega crodriguezvega marked this pull request as ready for review June 16, 2023 06:50
@@ -35,6 +35,9 @@ message MsgRegisterInterchainAccount {

// MsgRegisterInterchainAccountResponse defines the response for Msg/RegisterAccount
message MsgRegisterInterchainAccountResponse {
option (gogoproto.equal) = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the equal directives needed? See discussion

Copy link
Contributor Author

@crodriguezvega crodriguezvega Jun 20, 2023

Choose a reason for hiding this comment

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

Is it ok if a remove them all in a separate PR addressing #3871?

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunate that we added them in the update messages since that will need to be fixed in a separate PR to 04-channel. I'd vote for removing them here if we decide to go with #3871 but not strongly opinionated on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just much easier for me to merge this as is and then delete all the occurrences in a separate PR, instead of now looking one by one at the places where I added them and delete them. 😓

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, can totally see that. Its the little OCD voices that are complaining with the fact that something is added only to be removed. I have zero issues going with the route thats easier for you.

Copy link
Contributor

@charleenfei charleenfei Jun 21, 2023

Choose a reason for hiding this comment

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

i can also help rm the occurences in this pr! i agree that it's probably better to remove them in this pr rather than opening a separate one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, @charleenfei!

@crodriguezvega crodriguezvega changed the title chore(api)!: add missing gogoproto directives chore(api)!: add missing/remove unnecessary gogoproto directive Jun 21, 2023
charleenfei and others added 2 commits June 21, 2023 11:39
# Conflicts:
#	modules/core/02-client/types/tx.pb.go
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.

grep finds nothing here, lgtm!

@crodriguezvega crodriguezvega merged commit 78be372 into main Jun 22, 2023
@crodriguezvega crodriguezvega deleted the carlos/add-gogoproto-directives branch June 22, 2023 06:36
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.

Consider removing gogoproto.equal option for protos
5 participants