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

types: make subscription ID String Cow. #594

Merged
merged 1 commit into from
Dec 10, 2021
Merged

Conversation

niklasad1
Copy link
Member

No description provided.

@niklasad1 niklasad1 requested a review from a team as a code owner December 8, 2021 16:30
impl<'a> SubscriptionId<'a> {
/// Convert `SubscriptionId<'a>` to `SubscriptionId<'static>` so that it can be moved across threads.
///
/// This can cause an allocation if the id is a string.
Copy link
Member Author

Choose a reason for hiding this comment

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

technically only if s is a str slice.

}

impl<'a> TryFrom<JsonValue> for SubscriptionId<'a> {
type Error = ();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would there be any use in making this more specific so that we can catch the "wrong value type" vs "value too large" errors below?

Copy link
Member Author

Choose a reason for hiding this comment

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

yupp,

@jsdw
Copy link
Collaborator

jsdw commented Dec 8, 2021

Offhand it looks like we just use SubscriptionId<'static> around the place; what's the advantage of using Cow? (I'm sure I'm missing something!)

@dvdplm
Copy link
Contributor

dvdplm commented Dec 8, 2021

A bit of motivation for this change would be good, the code looks fine but I'm not sure why we need this.

Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

LGTM, not sure it's entirely necessary given we should be using numbers anyway :)

@niklasad1
Copy link
Member Author

LGTM, not sure it's entirely necessary given we should be using numbers anyway :)

Yeah, it's compat stuff for other libraries (e.g. jsonrpc crate) but not sure if we need anymore so maybe better just to use u64?!
I would love to remove that code :)

@niklasad1
Copy link
Member Author

niklasad1 commented Dec 8, 2021

Offhand it looks like we just use SubscriptionId<'static> around the place; what's the advantage of using Cow? (I'm sure I'm missing something!)

Yeah you are correct, I didn't manage to get rid of static lifetimes in the client code because we the send these over the channel and store them in a HashMap so won't work without further refactoring. So needless currently but I was annoyed that we have Id which is Cow and SubscriptionID is not Cow :)

however, this is not useful without further refactoring I guess besides to TryFrom instead of de-serializing SubscriptionID from serde::Value

@dvdplm dvdplm merged commit 59925c0 into master Dec 10, 2021
@dvdplm dvdplm deleted the types-subscription-id-cow branch December 10, 2021 12:01
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.

5 participants