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.

Sign up for GitHub

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

Add getter for grpc fallback urls #47

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Add getter for grpc fallback urls #47

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84ec0a9
Add getter for grpc fallback urls
Nomad-Free-Talent Oct 22, 2024
80c56be
Update visibility of grpc fallback urls
Nomad-Free-Talent Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/cosmos/src/cosmos_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,14 @@ impl CosmosBuilder {
self.grpc_fallback_urls.push(url.into().into());
}

/// gRPC fallback URLs
pub fn grpc_fallback_urls_ref(&self) -> Vec<&String> {
Copy link
Member

Choose a reason for hiding this comment

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

This approach forces an allocation of a Vec, most likely unnecessarily. I see a few alternative return types:

  • &Vec<Arc<String>>: most direct, loses no power versus this return type, but exposes internals of the library (the presence of the Arc). Given that we aren't provided API stability right now, I don't see a problem with that last part.
  • &[Arc<String>]: very similar to the above. I've actually never figured out whether it's more idiomatic to return a &Vec or &[] from a function. (For input, it's always better to take the slice to make the API more general.)
  • impl Iterator<&str>: best for hiding the internal API, doesn't add any cost, but slightly less flexible on the calling side

I slightly lean towards the first option. @psibi what do you think?

Copy link
Contributor Author

@Nomad-Free-Talent Nomad-Free-Talent Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In case we choose the first option, there's no need to have this additional method but change the visibility of the grpc_fallback_urls method.

cosmos-rs/packages/cosmos/src/cosmos_builder.rs

Lines 123 to 125 in 207c67a

pub(crate) fn grpc_fallback_urls(&self) -> &Vec<Arc<String>> {
&self.grpc_fallback_urls
}

What do you think @snoyberg ?

Copy link
Member

Choose a reason for hiding this comment

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

It's definitely a true statement. However, the advantage of keeping the field private is it gives more flexibility in the future to update the API. But given my other comments about not providing API compatibility, it's not strictly necessary. I kept it that way up until now because it was easy enough to provide the getter/setter approach. I'm not convinced it's worth it, so certainly open to changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case I'm gonna change the visibility of grpc_fallback_urls method and remove the current change.

self.grpc_fallback_urls
.iter()
.map(|url| url.as_ref())
.collect()
}

pub(crate) fn grpc_fallback_urls(&self) -> &Vec<Arc<String>> {
&self.grpc_fallback_urls
}
Expand Down
Loading