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

refactor: rename Provider method switchUrl to connect #1294

Merged
merged 16 commits into from
Sep 28, 2023

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf commented Sep 25, 2023

This PR:

  • renames the Provider method switchUrl to connect.
  • ensures new chain and node data is always fetched when instantiating a new Provider or when updating the Provider url ( connect

@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
84.93% (+0.09% 🔼)
5172/6090
🟡 Branches
66.4% (+0.12% 🔼)
759/1143
🟡 Functions 75.09% 850/1132
🟢 Lines
84.81% (+0.1% 🔼)
4953/5840
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / operations.ts
82.42% (-1.1% 🔻)
100%
46.67% (-3.33% 🔻)
82.42% (-1.1% 🔻)

Test suite run success

1237 tests passing in 213 suites.

Report generated by 🧪jest coverage report action from 4cae557

@arboleya
Copy link
Member

@LuizAsFight @luizstacio FWIW, I was the one who suggested renaming the connect method to switchUrl during my reviews with @Dhaiwat10blame should be given where the blame is due. ✋

The reasoning against the connect terminology is that the provider doesn't seem to connect to anything, so much so that we have no disconnect method. Unlike a Database/Socket connection that manages connection pools, TTL, handshakes, etc., it only serves as a bridge/proxy for fetching remote data. This is my understanding, and if we draw inspiration from standard Wallet dropdowns, an improved choice could be selectNetwork(url: string).

I have zero attachment to this, so feel free to do as you please.

arboleya
arboleya previously approved these changes Sep 26, 2023
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Approving this as-is, because I know it's been an external request.

However, I left my reasoning behind some points that [I believe] could be improved.

@nedsalk nedsalk self-requested a review September 27, 2023 06:50
Copy link
Contributor

@nedsalk nedsalk left a comment

Choose a reason for hiding this comment

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

Requested changes based on this discussion. Other than that, LGTM.

Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

I didn't know it changed, no opinion on the name itself. But yea it might have broken developer code.

LGTM

@Torres-ssf Torres-ssf merged commit 80e5381 into master Sep 28, 2023
@Torres-ssf Torres-ssf deleted the st/refactor/rename-provider-switch-url-to-connect branch September 28, 2023 12:07
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.

6 participants