-
Notifications
You must be signed in to change notification settings - Fork 35
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
Unify metadata version type and remove nodes config singleton #1200
Conversation
4b91af9
to
699201d
Compare
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.
Thanks for creating this PR @AhmedSoliman. The changes look good to me. +1 for merging.
I left a minor comment regarding the removal of NetworkAddress
. I have a small PR where I used this type to instruct the node server to bind to unix domain sockets or tcp sockets. There it would still be useful.
@@ -179,59 +84,52 @@ impl NodeConfig { | |||
|
|||
#[derive(Debug, Clone, Eq, PartialEq, Hash, derive_more::Display)] | |||
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
pub enum NetworkAddress { |
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.
Something like the NetworkAddress
can still be useful to instruct the node server where to bind to. I guess it could contain the variants Uds
and TcpSocketAddr
, for example.
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.
Discussed in #1201
Unify metadata version type and remove nodes config singleton