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

use ecosystem convention for keeper names in app.go #882

Closed
wants to merge 2 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jun 2, 2022

This PR changes keeper names in app.go to match the commonly used ecosystem conventional style, which:

  • Makes wasmd easier to reference
  • Makes upgrading wasmd easier, because there will be fewer merge conflicts

@faddat
Copy link
Contributor Author

faddat commented Jun 2, 2022

My preference here is to first:

The specific reason is that go 1.18 will change some of the supported linters. We could then dial in on the ones that we think are valuable, and also exclude anything that isn't.

@alpe
Copy link
Member

alpe commented Jun 3, 2022

Thanks for bringing this up! 🌻

I have seen public fields in other apps, too. I was always wondering why break encapsulation without a need.
I can see public fields being helpful with the test suite pattern used in sdk and elsewhere. Do you have other examples?

Makes wasmd easier to reference

what do you mean with this?

@faddat
Copy link
Contributor Author

faddat commented Jun 3, 2022

@alpe - the practice that I openly recommend to other developers when constructing app.go is to very literally copy and paste. Being generally the same as other chains makes that easier.

re: encapsulation, and having a standard on these-- I have wondered same as yourself.

from the perspective of someone working on Juno, who'd like to move changes between wasmd and juno, adopting this standard would make upstreaming work into wasmd easier.

@ethanfrey
Copy link
Member

from the perspective of someone working on Juno, who'd like to move changes between wasmd and juno, adopting this standard would make upstreaming work into wasmd easier.

I agree 💯 with you that we should have a standard and the same format in wasmd, osmosis and Juno.

However, Alex's question was why Juno's approach is better than the wasmd approach. Maybe you or another Juno dev can explain why these are needed to be public. (I have some ideas, but I prefer feedback from actual users)

@faddat
Copy link
Contributor Author

faddat commented Jul 8, 2022

@ethanfrey honestly I don't see a reason in either direction, 🤣

I think this is purely about following convention and making merges easier.

@faddat
Copy link
Contributor Author

faddat commented Jul 8, 2022

OK, this one is ready to rock. After merging this and #815, the review scope for #863 will drop a great deal.

@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #882 (df44859) into main (4525d51) will not change coverage.
The diff coverage is 78.97%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #882   +/-   ##
=======================================
  Coverage   59.14%   59.14%           
=======================================
  Files          51       51           
  Lines        6198     6198           
=======================================
  Hits         3666     3666           
  Misses       2265     2265           
  Partials      267      267           
Impacted Files Coverage Δ
app/test_access.go 0.00% <0.00%> (ø)
app/test_helpers.go 0.84% <0.00%> (ø)
app/export.go 12.12% <4.16%> (ø)
x/wasm/keeper/ibc.go 77.77% <33.33%> (ø)
x/wasm/module.go 49.45% <66.66%> (ø)
app/app.go 88.91% <98.62%> (ø)
x/wasm/keeper/handler_plugin.go 85.14% <100.00%> (ø)
x/wasm/keeper/keeper.go 88.23% <100.00%> (ø)

@ethanfrey ethanfrey added this to the v0.29.0 milestone Aug 15, 2022
@alpe alpe mentioned this pull request Aug 23, 2022
@alpe
Copy link
Member

alpe commented Aug 23, 2022

Thanks for the PR and bringing this topic up! I was pending for a while now due to my other priorities but I polished your commits in #951.

@alpe alpe closed this Aug 23, 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.

None yet

3 participants