[WIP] Refactor of a refactor?

alexkornitzer commented 5 years ago

I saw that there was a refactor (0.3.0) going on for this project and understood the benefits of that over 0.2.0, mostly the move to reqwest, etc. That being said there were certain parts that I did not understand the decisions for until looking at HashiCorp's implementation of the API in go. While I then understood the decisions, I did not agree with this go style implementation, so I have converted it to a more rust like implementation. There are quire a few changes here, with not all of them finished hence the WIP, so I will try to summarise them below:

[x] Move to failure from error-chain, as it is deprecated
[x] Port structures and their names from HashiCorp's go implementation
[x] Use serde's renaming feature to enable the following of Rust's style guides
[x] Remove bloat from the API and their requests
[x] Implement a blocking trait for those interested in blocking queries
[x] Add a Config builder
[x] Add a custom Reqwest builder
[x] Implement blocking trait for KV
[x] Add additional API
[x] Remove unnecessary Options
[x] Remove go style tuples
[ ] Test all API routes,
[ ] Provide sufficient integration tests
[ ] Add documentation
[ ] Implement remaining blocking APIs
[ ] Use macros and custom derives to reduce code duplication and improve maintainability

Hopefully these changes make sense and are of use to you, there are definitely further improvements to the design that can be added here but I have got it to a stage where I can use it easily within the project that I was working on.

stusmall commented 5 years ago

Thank you so much for this. I'll take a look through it tonight. Like you said, the logic for what I started was to get it closer to the hashicorp driver. While it makes an awkward rust interface it helped speed the implementation without running into design surprises that caused me to go back and rework large sections of the implementation.

stusmall commented 5 years ago

Sorry about the extremely slow reply on this. This looks great, especially the serialization and error handling changes. I understand why returning the tuple with the meta is un-rustic. If it is dropped off how will a user access X-Consul-Index? It is needed for blocking queries.

The go API passes other values via these Meta structs but this is the only one I've supported so far.

alexkornitzer commented 5 years ago

No worries, right so both go and python return tuples with indexes, etc. For rust I thought implementing blocking as a trait could be a better approach, but this is probably one of preference: https://github.com/stusmall/consul-rust/pull/20/files#diff-b4aea3e418ccdb71239b96952d9cddb6R139

I saw that the go client passes a lot of meta but imo they all seemed like information that should be implemented by the library user if wanted, i.e. query times.

stusmall commented 5 years ago

I've ended up merging in the refactor branch into master but weill still be watching work here

pierresouchay / consul-rust

[WIP] Refactor of a refactor? #20