bitcoindevkit / rust-esplora-client

Bitcoin Esplora API client library. Supports plaintext, TLS and Onion servers. Blocking or async.
MIT License
28 stars 44 forks source link

refactor(async): refactoring async client and minor documentation improvements #93

Open oleonardolima opened 3 weeks ago

oleonardolima commented 3 weeks ago

Description

It builds on top of #95. Applies minor improvements on some docstrings, and Cargo.toml standard.

The main change is adding the common HTTP methods for the AsyncClient, it removes duplicated code from each Esplora API request, and follows the approach done for BlockingClient.

It makes it easier to extract these methods into an AsyncEsploraClient trait (to be done in another PR, initially done here 9cbc3873c2a56258e13cb36e0e8c32039f0947a7), which the user can implement with any HTTP client of its choice. Also, makes it simpler to rebase and update the AsyncAnonymizedClient from #67.

Notes to the reviewers

It has some commits from #95, as it builds on top of it and should be merged afterward. Please let me know what you think about the proposed changes and approach.

Changelog notice

Checklists

All Submissions:

coveralls commented 3 weeks ago

Pull Request Test Coverage Report for Build 10530778457

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/async.rs 102 121 84.3%
<!-- Total: 104 123 84.55% -->
Files with Coverage Reduction New Missed Lines %
src/lib.rs 1 97.08%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 10514244019: 1.3%
Covered Lines: 1014
Relevant Lines: 1142

💛 - Coveralls
ValuedMammal commented 2 weeks ago

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

oleonardolima commented 1 week ago

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think

I think it could be helpful to get exposed in async/reqwest too, I'll check it.

ValuedMammal commented 1 week ago

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think

I think it could be helpful to get exposed in async/reqwest too, I'll check it.

Ah ok so maybe not necessary since you can already get a handle to the reqwest::Client?

oleonardolima commented 1 week ago

A recent PR #87 exposed a get_request method for the blocking client that returns a new Request, would you want to do a similar thing for the async client?

IIUC exposes a get_request because it does not have a minreq client as a field on BlockingClient, as minreq does not have this "strategy" as reqwest has :think I think it could be helpful to get exposed in async/reqwest too, I'll check it.

Ah ok so maybe not necessary since you can already get a handle to the reqwest::Client?

Yes, that's what I thought.

oleonardolima commented 1 week ago

ACK 9cb662a

Overall this looks great.

In 9cb662a

refactor(async)!: add common GET and POST methods

Maybe I missed it but I didn't see how this change is API breaking in any way?

Sweet! I updated the commit message, and add a new one applying the same style on other methods.

oleonardolima commented 1 week ago

I rebased this one on top of master, as #95 got merged. It should be ready to go now.

oleonardolima commented 1 week ago

fyi: I'm attempting to fix the code coverage CI step issue on #96.

evanlinjin commented 16 hours ago

@oleonardolima since it seems we are planning to go down the #97 route, is this PR still relevant?

oleonardolima commented 7 hours ago

@oleonardolima since it seems we are planning to go down the #97 route, is this PR still relevant?

I guess this one can still land if we'd like to cut a release with these improvements, and mainly with #98. Then, cut a new release specifically with the refactor of #97. 🤔