Allow host to include https

schlunsen commented 2 years ago

Make it optional if the host should include the http protocol, i.e https://bitcoind-host instead of bitcoind-host

schlunsen commented 2 years ago

This is useful if you connect to a bitcoind node via a ssl tunnel

bibajz commented 2 years ago

Hello,

thank you for the pull request!

I was thinking about the issue and I believe the initial design is not really good:

as is the case of your PR, parameters host, port, which are then transformed to a URL, are not really sufficient - it should have been url parametr all along, dropping the _set_url method
during initialization of BitcoinRPC, you cannot pass an already initialized instance of httpx.AsyncClient
httpx.AsyncClient initialization in _configure_client method is kinda clunky, so maybe rework that too

All in all, there's work to be done here. :D

In the following days I will take a closer look on the issues outlined above and will create some pull requests, sounds good @schlunsen ?

Thanks again and cheers! Libor

schlunsen commented 2 years ago

Your proposals makes sense and the pull request was done in rush, so I really appreciate your work here of thinking it through.

Thanks for the project!

bibajz commented 2 years ago

OK, I addressed the url thing in #11 , can you take a short look at it? :)

If you find it cool, I will merge it and release a new version soon after!

Oh and by the way, I hope you understood my remark, but just to clarify - my point about a poor design in https://github.com/bibajz/bitcoin-python-async-rpc/pull/10#issuecomment-1001560293 was aimed at my design choice of the overall BitcoinRPC class, not your pull request :)

Cheers, Libor

bibajz / bitcoin-python-async-rpc