-
Notifications
You must be signed in to change notification settings - Fork 210
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
chore: remove unneeded usage of pub
throughout gateway code
#4357
Conversation
pub
throughout gateway code
pub
throughout gateway codepub
throughout gateway code
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 in this case it's ok to reduce visibility, but @douglaz made a good point yesterday that making things pub again when needed is a lengthy process. So maybe for now we only want to make things private where it's necessary for guaranteeing invariants.
@@ -1165,7 +1165,7 @@ impl Gateway { | |||
/// - `GatewayConfiguration` is read from the database. | |||
/// - All cli or environment variables are set such that we can create a | |||
/// `GatewayConfiguration` | |||
pub async fn get_gateway_configuration(&self) -> Option<GatewayConfiguration> { | |||
async fn get_gateway_configuration(&self) -> Option<GatewayConfiguration> { |
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.
Is this something you might want to call if you instantiate the gateway yourself as part of a custom binary?
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'd say yes, but right now I don't see us supporting custom binaries for gateways
@@ -240,7 +239,7 @@ impl ClientModule for GatewayClientModule { | |||
} | |||
|
|||
impl GatewayClientModule { | |||
pub fn to_gateway_registration_info( | |||
fn to_gateway_registration_info( |
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.
Maybe useful for integrators
@douglaz do you think this PR is worth it? If so it needs a rebase. |
Rebased! |
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.
Send it
No description provided.