-
Notifications
You must be signed in to change notification settings - Fork 27
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
Subsystems configuration #277
Conversation
I'm still not sure about what approach would be better here: split the current
|
Finally ready to review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is OK. There are two things that I feel can be improved:
- There's no configuration reading for ChainConfig. There has to be a way to configure ChainConfig itself for functional tests. But this can be made in another PR.
- The initialization of the config can be improved. All these
if let
s are very ugly. One solution is to create a wrapper type with a generic trait for a NodeConfigEntry. Then you implement that for all types that can be taken from config (ints, strings, etc). Finally, you can make a method there to load values fromOption<T>
if a value exists. Maybe you can come up with something else that's simpler.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good now, besides the minor comments I mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Rather unfortunate there are separate mechanisms for dealing with options coming from a config file and options coming from the command line with this manual translation, but given that, the solution presented here looks rather nice.
The goals are the following: