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

feat: Add option to set a request timeout #81

Merged
merged 4 commits into from May 5, 2024

Conversation

threema-donat
Copy link
Contributor

Description

Adds the option to set a timeout on requests. For that, the existing options endpoint and signer are moved to a new ClientOptions struct that also allows setting the pool and request timeout (in seconds).

Resolves #73

How Has This Been Tested?

This has not yet been tested since my testing framework of choice (wiremock) does not support setting a timeout.

Due Dilligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

src/client.rs Outdated Show resolved Hide resolved
@chris13524
Copy link
Member

Please resolve conflicts :)

Copy link
Member

@chris13524 chris13524 left a comment

Choose a reason for hiding this comment

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

Some more suggestions on how to further refine these options into a full builder pattern:

  • Rename ClientOptions to ClientBuilder
  • Remove endpoint param from ClientBuilder::new() and default to Production
  • Add builder() -> ClientBuilder method to Client
  • Add build(self) method to ClientBuilder which will be the only mechanism to construct Client
  • Remove params from Client::new() and implement with with ClientBuilder::new().build(). Delete Client::new_with_defaults()

Another example

What do you think?

@threema-donat
Copy link
Contributor Author

Some more suggestions on how to further refine these options into a full builder pattern:

* Rename `ClientOptions` to `ClientBuilder`

* Remove `endpoint` param from `ClientBuilder::new()` and default to `Production`

* Add `builder() -> ClientBuilder` method to `Client`

* Add `build(self)` method to `ClientBuilder` which will be the only mechanism to construct `Client`

* Remove params from `Client::new()` and implement with with `ClientBuilder::new().build()`. Delete `Client::new_with_defaults()`

Another example

What do you think?

I think it makes definitely sense there, but for passing the arguments from the user to the library I would suggest to have a simple struct since a builder for three fields is rather too complex. I added a ClientConfig::new convenience fn since most users probably want to configure this but not the other fields.

@threema-donat threema-donat force-pushed the add_timeout branch 2 times, most recently from 605427b to 840c450 Compare April 30, 2024 09:42
builder.http2_only(true);
#[derive(Debug, Clone)]
/// The default implementation uses [`Endpoint::Production`] and can be created
/// trough calling [`ClientConfig::default`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// trough calling [`ClientConfig::default`].
/// through calling [`ClientConfig::default`].

@chris13524 chris13524 merged commit 8cd21fc into WalletConnect:master May 5, 2024
5 checks passed
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.

How to set timeout on send?
2 participants