search

wisespace-io / binance-rs

Rust Library for the Binance API
Other
649 stars 295 forks source link

Creation of config options #75

Closed dorak88783 closed 3 years ago

dorak88783 commented 3 years ago

To solve https://github.com/wisespace-io/binance-rs/issues/61 and https://github.com/wisespace-io/binance-rs/issues/74, we need a Config struct. This was my first attempt to make one.

I made a new new_with_config function in the Binance trait. We could also change the signature of the existing new function, but that would be an API breaking change. @wisespace-io: Please let me know what you think is best.

This PR was for early feedback, what I want to do for sure is:

All feedback welcome!

dorak88783 commented 3 years ago

I added a testnet moethod and added futures.

It didn't look much nicer with &str instead of String, since the Client struct also uses String, so you'd have to convert somewhere anyway, so I kept it like it was.

I'm finished coding; it works fine for me like this. I'd appreciate feedback.

wisespace-io commented 3 years ago

@dorak88783 I think new_with_config is a good start. We can get some feedback when people start using it and we make it the default way in a near future. I am aware that we have a design issue with &str and String. When I created this project I was starting in the Rust world. It can be improved.

dorak88783 commented 3 years ago

Thanks! It is an impressive project for somebody starting with Rust :)

The &str vs String is not a big deal for me ; it's just nicer to be able write .set_rest_api_endpoint("https://testnet.binance.vision/api") instead of .set_rest_api_endpoint("https://testnet.binance.vision/api".into()). In terms of performance it doesn't matter that much...

I also thought about adding the API key and API secret to the config/builder pattern, but decided not to do this yet. Maybe this all can be unified in a future (API breaking) version.

dorak88783 commented 3 years ago

Ah I could fix the syntax using this Into<String> type.

Maybe a String is a good type for Config and Client anyway as it's clear that they own this.

wisespace-io commented 3 years ago

@dorak88783 I just finished the tests. It seems to work fine. Merging it now. Thank you for the contribution.

thetrung commented 2 years ago

Why can't I access both new_with_config and Config struct from current crate ? Is it still hidden ?