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

lint legacy ibc testing #1078

Merged
merged 9 commits into from
Aug 15, 2023
Merged

lint legacy ibc testing #1078

merged 9 commits into from
Aug 15, 2023

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 23, 2023

I apologize, because gh has no such preview tab.

This PR lints the legacy_ibc_testing folder.

Please go the the Preview tab and select the appropriate sub-template:

  • Production code - for types fix, feat, and refactor.
  • Docs - for documentation changes.
  • Others - for changes that do not affect production code.

@faddat faddat requested a review from a team as a code owner June 23, 2023 12:11
@@ -36,7 +36,7 @@ func ReconstructPacketFromEvent(event abci.Event) (packet types.Packet, err erro
return packet, err
}
return types.NewPacket(
attrMap[string(types.AttributeKeyData)], // data
attrMap[string(types.AttributeKeyDataHex)], // data
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do? Doesn't seem like a lint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

old call is deprecated and I think that these both return the same thing

@@ -69,7 +69,7 @@ func ParseAckFromEvents(events sdk.Events) ([]byte, error) {
for _, ev := range events {
if ev.Type == channeltypes.EventTypeWriteAck {
for _, attr := range ev.Attributes {
if string(attr.Key) == channeltypes.AttributeKeyAck {
if attr.Key == channeltypes.AttributeKeyAckHex {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above. Maybe the two changes are tied together but I'm evaluating this as a linting / fully refactor PR

@shaspitz
Copy link
Contributor

These changes seem to make tests fail

@faddat
Copy link
Contributor Author

faddat commented Jun 25, 2023

ah, you know, that explains some stuff. Also, it is in the exact location you flagged -- I thought the two calls did the same thing.

Thanks!

@faddat
Copy link
Contributor Author

faddat commented Jun 25, 2023

PS: do you know where to find the preview tab?

@mpoke
Copy link
Contributor

mpoke commented Jul 6, 2023

PS: do you know where to find the preview tab?

@faddat When you open a PR using the github UI (https://github.com/cosmos/interchain-security/compare), you have a Write tab, which is selected by default. Next to it, there is the Preview tab.

@faddat
Copy link
Contributor Author

faddat commented Jul 6, 2023

Thanks @mpoke -- is it posssible to settle on one template?

I'll fix this one up manually, but basically here's my usual workflow, which is why you're still sometimes seeing this from me:

git checkout origin/main
git switch -c faddat/branchname
# do stuff
git commit -a
gh pr create --draft
# do more stuff
git commit -a
gh pr ready 1078

Copy link
Contributor

@jtremback jtremback left a comment

Choose a reason for hiding this comment

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

LGTM

@mpoke
Copy link
Contributor

mpoke commented Jul 18, 2023

Thanks @mpoke -- is it posssible to settle on one template?

Having different templates for different contributions types helps with the workflow. For example, a bug fix has different requirements than an update of the docs.

I'll fix this one up manually, but basically here's my usual workflow, which is why you're still sometimes seeing this from me:

git checkout origin/main
git switch -c faddat/branchname
# do stuff
git commit -a
gh pr create --draft
# do more stuff
git commit -a
gh pr ready 1078

I'm not that familiar with the gh CLI command, but here is a related issue cli/cli#2567.

@shaspitz
Copy link
Contributor

@faddat please run make lint or make format so the golangci-lint test can pass. Then this is ready to merge, thanks

@faddat
Copy link
Contributor Author

faddat commented Aug 13, 2023

awesome, just a moment sir.

@shaspitz shaspitz merged commit 361277c into cosmos:main Aug 15, 2023
9 checks passed
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.

4 participants