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

refactor: remove Request suffix in Msgs #422

Merged
merged 7 commits into from
Jul 21, 2021
Merged

Conversation

atheeshp
Copy link
Contributor

@atheeshp atheeshp commented Jul 16, 2021

Description

Closes: #399


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)

@atheeshp atheeshp changed the title refactor: remove request suffix refactor: remove Request suffix in Msgs Jul 16, 2021
@atheeshp atheeshp marked this pull request as ready for review July 16, 2021 09:17
@atheeshp atheeshp requested a review from a team July 16, 2021 09:17
Copy link
Contributor

@ruhatch ruhatch left a comment

Choose a reason for hiding this comment

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

LGTM!

@amaury1093
Copy link
Contributor

Tests still need to be fixed. We can safely ignore the Protobuf / breakage CI check here

@aaronc
Copy link
Member

aaronc commented Jul 19, 2021

Has anyone looked into the test coverage issue here?

@atheeshp
Copy link
Contributor Author

Has anyone looked into the test coverage issue here?

I'm trying to fix this.

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #422 (c8b9562) into master (0bd4cd9) will decrease coverage by 0.03%.
The diff coverage is 22.22%.

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
- Coverage   56.63%   56.60%   -0.04%     
==========================================
  Files          59       59              
  Lines        3752     3754       +2     
==========================================
  Hits         2125     2125              
- Misses       1356     1357       +1     
- Partials      271      272       +1     
Flag Coverage Δ
experimental-codecov 56.60% <22.22%> (-0.04%) ⬇️
stable-codecov 56.60% <22.22%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@blushi blushi self-assigned this Jul 20, 2021
@atheeshp atheeshp requested a review from aaronc July 20, 2021 15:17
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Pre-approving, I left one comment that is good to keep in mind for the future but is out of scope of this PR.

@@ -639,6 +639,7 @@ github.com/otiai10/mint v1.3.0/go.mod h1:F5AjcsTsWUqX+Na9fpHb52P8pcRX2CI6A3ctIT9
github.com/otiai10/mint v1.3.2 h1:VYWnrP5fXmz1MXvjuUvcBrXSjGE6xjON+axB/UrpO3E=
github.com/otiai10/mint v1.3.2/go.mod h1:/yxELlJQ0ufhjUwhshSj+wFjZ78CnZ48/1wtmBH1OTc=
github.com/pact-foundation/pact-go v1.0.4/go.mod h1:uExwJY4kCzNPcHRj+hCR/HBbOOIwwtUjcrb0b5/5kLM=
github.com/pariz/gountries v0.0.0-20200430155801-1c6a393df9c7/go.mod h1:U0ETmPPEsfd7CpUKNMYi68xIOL8Ww4jPZlaqNngcwqs=
Copy link
Member

Choose a reason for hiding this comment

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

why is this here?

Comment on lines +81 to +88
ecocreditGenesisState := ecocredit.DefaultGenesisState()
ecocreditGenesisBytes, err := cdc.MarshalJSON(ecocreditGenesisState)
require.NoError(err)

genesisData := map[string]json.RawMessage{
group.ModuleName: genesisBytes,
ecocredit.ModuleName: ecocreditGenesisBytes,
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not so fond of having to put ecocredit module related logic in the group module tests but I guess we have no other choice with the current setup, unless we create public functions InitGenesis and ExportGenesis that could be tested individually without the server (they would be used in serverImpl.InitGenesis and serverImpl.ExportGenesis) similarly to what we have with the invariants tests. But then that would force us to set up all the tables again in the tests while this is already handled by the integration test suite here so this is not ideal either. So let's keep it as is for now until we come to a better solution in terms of module wiring / test setup.

@aaronc aaronc merged commit c99dbed into master Jul 21, 2021
@aaronc aaronc deleted the atheesh/remove-req-suffix branch July 21, 2021 16:32
@atheeshp atheeshp restored the atheesh/remove-req-suffix branch July 26, 2021 09:14
@ryanchristo ryanchristo deleted the atheesh/remove-req-suffix branch July 2, 2022 22:09
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.

Remove Request suffix from Msg types in x/ecocredit, x/data and x/group
6 participants