Use the esplora client crate #764
Conversation
Cargo.toml
Outdated
| use-esplora-reqwest = ["esplora", "reqwest", "reqwest/socks", "futures"] | ||
| use-esplora-ureq = ["esplora", "ureq", "ureq/socks"] | ||
| use-esplora-reqwest = ["esplora", "esplora-client/async", "futures"] | ||
| use-esplora-ureq = ["esplora", "esplora-client/blocking"] |
There was a problem hiding this comment.
I think keeping these names is a bit strange given the ureq/reqwest dependencies have been removed. Maybe these features could also be renamed use-esplora-async/use-esplora-blocking, while initially keeping the deprecated features around as aliases to not break compatibility?
There was a problem hiding this comment.
I also like this idea. Add the new feature names, but keep the old ones around for a release with comment in Cargo.toml and we'll put in tag changelog that old feature names are deprecated.
There was a problem hiding this comment.
Apparently there's no way to deprecate a feature, should we just add a note to the changelog?
There was a problem hiding this comment.
What you did with making the old ureq/reqwest features aliases for the new names is all I was expecting, plus adding your change log note.
There was a problem hiding this comment.
In line with @tnull point, I was wondering if it doesn't also make sense to rename the ureq.rs and reqwest.rs file under the esplora module to blocking.rs and async.rs respectively.
There was a problem hiding this comment.
I did consider it but since the modules are not public it doesn't make any difference for the public API, so it's just an internal thing
There was a problem hiding this comment.
Hm, but doesn't that mean that there is even less of a reason not to clean it up?
There was a problem hiding this comment.
Yeah, maybe, I personally don't mind but I'm not against cleaning it up either.
I replied like that because for some reason I though this PR had already been merged by Steve and so I thought it was too late to fix it and I didn't want to open another PR just for that. But since this is still open yeah I can do it!
There was a problem hiding this comment.
I'll do a quick re-review and merge it now. :-)
afilini
force-pushed
the
feat/use-external-esplora-client
branch
4 times, most recently
from
7cee104
to
0e84361
Compare
afilini
force-pushed
the
feat/use-external-esplora-client
branch
from
0e84361
to
5f50be9
Compare
afilini
force-pushed
the
feat/use-external-esplora-client
branch
from
5f50be9
to
d7bfe68
Compare
Description
Use the external esplora client crate now that it's published
Changelog notice
use-esplora-reqwestanduse-esplora-ureqfeatures in favor ofuse-esplora-asyncanduse-esplora-blockingChecklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: