Skip to content

chore(provider): use WeakClient in GetSubscription #2219

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

Merged
merged 12 commits into from
Mar 20, 2025

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Mar 19, 2025

Motivation

The GetSubscription future type was introduced in #2203
Remove it's reliance on N: Network by using WeakClient

Solution

  • Remove N: Network generic from GetSubscription by using WeakClient

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

nice,

some nits

@github-project-automation github-project-automation bot moved this to In Progress in Alloy Mar 19, 2025
@yash-atreya yash-atreya marked this pull request as ready for review March 19, 2025 11:12
@yash-atreya yash-atreya self-assigned this Mar 19, 2025
@yash-atreya yash-atreya moved this from In Progress to Ready for Review in Alloy Mar 19, 2025
@github-project-automation github-project-automation bot moved this from Ready for Review to In Progress in Alloy Mar 19, 2025
@yash-atreya yash-atreya changed the title feat(provider): apply GetSubscription to trait chore(provider): use WeakClient in GetSubscription Mar 19, 2025
@yash-atreya yash-atreya requested a review from mattsse March 19, 2025 16:13
@yash-atreya yash-atreya moved this from In Progress to Ready for Review in Alloy Mar 19, 2025

fn into_future(self) -> Self::IntoFuture {
Box::pin(async move {
let pubsub = self.root.pubsub_frontend()?;
let client =
self.client.upgrade().ok_or(TransportErrorKind::custom_str("client dropped"))?;
Copy link
Member

Choose a reason for hiding this comment

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

ok_or_else

Copy link
Member Author

@yash-atreya yash-atreya Mar 20, 2025

Choose a reason for hiding this comment

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

Isn't ok_or cleaner?
Regarless, it's a nit 5a8d29f

@github-project-automation github-project-automation bot moved this from Ready for Review to In Progress in Alloy Mar 20, 2025
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this needs a quick followup with the changes in #2220

@yash-atreya yash-atreya requested a review from DaniPopes March 20, 2025 10:30
@yash-atreya yash-atreya enabled auto-merge (squash) March 20, 2025 10:34
@yash-atreya yash-atreya disabled auto-merge March 20, 2025 10:36
@github-project-automation github-project-automation bot moved this from In Progress to Reviewed in Alloy Mar 20, 2025
@yash-atreya yash-atreya merged commit e45a0f9 into main Mar 20, 2025
27 checks passed
@yash-atreya yash-atreya deleted the yash/apply-get-subs branch March 20, 2025 12:22
@github-project-automation github-project-automation bot moved this from Reviewed to Done in Alloy Mar 20, 2025
@grandizzy grandizzy moved this from Done to Completed in Alloy Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

3 participants