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(api, blocking, async)!: make library generic over any HTTP client #99

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oleonardolima
Copy link
Collaborator

@oleonardolima oleonardolima commented Sep 15, 2024

fixes #97

Description

It refactors the library, introducing and adding new structures/enums for Esplora API: TransactionApi, AddressesApi, BlocksApi, and FeeEstimateApi.

Introduce and exposes new trait Client with minimal HTTP behaviors: request, send, send_async, and deserialize_{decodable|json|str} methods.

Any HTTP client can now implement either the send or send_async method, use it's inner HTTP client structure for dispatching and handling the request/response, then use the common already implemented methods for deserializing the response.

Notes to the reviewers

I still think it's not there yet, the ergonomics and usage can be improved, but I'd like to see if anyone has any comments or suggestions on the approach.

Changelog notice

TBD

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@oleonardolima oleonardolima added the enhancement New feature or request label Sep 15, 2024
@oleonardolima oleonardolima self-assigned this Sep 15, 2024
@oleonardolima oleonardolima force-pushed the refactor/make-library-generic-over-any-http-client branch 3 times, most recently from 3e0123f to 017a0cc Compare September 15, 2024 23:17
@coveralls
Copy link

coveralls commented Sep 15, 2024

Pull Request Test Coverage Report for Build 10894551662

Details

  • 440 of 521 (84.45%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.5%) to 89.032%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/blocking.rs 99 118 83.9%
src/async.rs 149 169 88.17%
src/api.rs 192 234 82.05%
Files with Coverage Reduction New Missed Lines %
src/async.rs 1 85.49%
src/blocking.rs 2 85.56%
Totals Coverage Status
Change from base Build 10748775132: 1.5%
Covered Lines: 1242
Relevant Lines: 1395

💛 - Coveralls

@oleonardolima oleonardolima force-pushed the refactor/make-library-generic-over-any-http-client branch from 017a0cc to 86863f4 Compare September 16, 2024 00:50
@oleonardolima
Copy link
Collaborator Author

@evanlinjin, I've started working on this one, on top of what you've suggested if you want to have a look.

I'm still thinking of ways to have better ergonomics on its usage, and not have the multiple deserialize_... methods.

@oleonardolima oleonardolima force-pushed the refactor/make-library-generic-over-any-http-client branch from 86863f4 to c3c46e4 Compare September 16, 2024 20:57
- add new APIs: `TransactionApi`, `AddressesApi`, `BlocksApi` and
  `FeeEstimatesApi`
- add new `trait Client` with minimal HTTP behaviors: `request`, `send`
  and `deserialize_{decodable|json|str}` methods.
- apply new APIs to `BlockingClient`, create new `handler` to build
  request and send with minreq HTTP client.
async

- add new `trait Client` behavior: `send_async`.
- apply new APIs to `AsyncClient`, create new `handler` to build request
  and send with `reqwest` HTTP client.
@evanlinjin
Copy link
Member

Thanks for the progress on this! I still need to have a proper look.

For now, I'm wondering whether we should just depend on the http crate for HTTP types. My argument is that hyper, ureq, reqwest all depend on it. minreq does not, but we can still convert from http types to minreq types, and I don't think we should promote it since it doesn't do H2.

@oleonardolima
Copy link
Collaborator Author

Thanks for the progress on this! I still need to have a proper look.

For now, I'm wondering whether we should just depend on the http crate for HTTP types. My argument is that hyper, ureq, reqwest all depend on it. minreq does not, but we can still convert from http types to minreq types, and I don't think we should promote it since it doesn't do H2.

Thanks! Sure, I'll update it to depend on the HTTP crate. It's a pretty slim dependency, and I don't see a problem with relying on it.

Agree, it's just a matter of implementing a From trait for the correspondent minreq (or any other http library) types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor library so that it can work with any HTTP client crate
3 participants