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

Ethans gconf comments #534

Merged
merged 13 commits into from
Apr 18, 2019
Merged

Conversation

ethanfrey
Copy link
Contributor

Code speaks a thousand docs.

I liked this approach. But made some changes. Please look at the diff.

  1. removed magic. pass in the package name/key rather than auto-querying it
  2. enforce validation on parsing
  3. abstracted out some basic InitConfig logic to make it easy to integrate in each package
  4. Ensure tests in gconf and x/cash pass
  5. changed genesis format to
"conf": {
  "cash": {..},
...

Please review the code and let me know what you think. If you are good, we can clean up and use this approach

@ethanfrey ethanfrey requested review from husio and alpe April 16, 2019 19:54
Copy link
Contributor

@husio husio left a comment

Choose a reason for hiding this comment

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

I like this approach most of all as well. Requiring an explicit package name instead of using reflect is a good change.

This functionality is not complete yet and I would love to see it used in bnsd as well to get the feeling for how it will be used.

@alpe what do you think?

gconf/gconf.go Outdated Show resolved Hide resolved
gconf/gconf.go Outdated Show resolved Hide resolved
@@ -35,6 +35,7 @@ message FeeInfo {


message Configuration {
// TODO: add schema uint32 here
Copy link
Contributor

Choose a reason for hiding this comment

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

This should land in #532 after merging. I think it's better to implement it before merging.

@ethanfrey
Copy link
Contributor Author

Yeah, I was a bit sloppy on the details. If Alex likes the idea as well, we can flesh it out. I would also add validation rules and see how to update schema/config before merging to master.

But if we agree this is the direction we go, we can start working on the details. In office today or remote

This was referenced Apr 17, 2019
@ethanfrey
Copy link
Contributor Author

@husio This branch should be good to go.

I added the generic handler with some basic tests and also fixed all tests in cmd/* and examples/mycoin. Please run make test locally

If you like this, please merge it into your PR, rebase that PR on newest master, and then add any more changes you want to do to consider this finished. I am sure there is plenty left to improve, but the basic flow should work and captures all difficult logic in gconf.

@ethanfrey
Copy link
Contributor Author

Oh, and this will need a nicely updated CHANGELOG before merging to master 😉

@husio husio merged commit 3f3b091 into cleanup_gconf_2_issue_456 Apr 18, 2019
@husio husio deleted the ethans-gconf-comments branch April 18, 2019 05:30
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.

2 participants