elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
695 stars 71 forks source link

Pass Transport to builder structs #107

Closed russcam closed 4 years ago

russcam commented 4 years ago

This PR proposes a breaking change to the client, to pass a reference to Transport to builder structs instead of a reference to Elasticsearch.

Builder structs only use the Transport of the client, calling its send() function, currently performed through Elasticsearch client's send() function. By passing the Transport directly, the send indirection is removed.

mwilliammyers commented 4 years ago

I am trying to envision what this looks like from an end-users perspective and its a bit tricky given the diffs 😄

Does this allow the omitting of send?

let client = Elasticsearch::default();

let search_response = client
    .search(SearchParts::None)
    .body(...)
    .allow_no_indices(true)
    .await?;
russcam commented 4 years ago

@mwilliammyers from the end users perspective, if APIs are called from client.*, then there is no difference. If users are instantiating builder structs independently of the client, only a Transport need be passed now, as opposed to Elasticsearch root client. I think this is better since an instance of a Transport is the only component required.

Does this allow the omitting of send?

Not right now. I thought about taking this approach in https://github.com/elastic/elasticsearch-rs/issues/1#issuecomment-559322511, but am personally undecided on it. As per the comment, it would mean moving where the data is constructed, which is currently in send(). The bit I'm undecided about is whether this is worthwhile to do to save on typing .send() 😄

mwilliammyers commented 4 years ago

@russcam ahhh, makes sense!

Not right now. I thought about taking this approach in #1 (comment), but am personally undecided on it.

I am in favor of keeping .send(). Like you said, I don't think it is worthwhile to save those extra characters. I think it's better to be explicit about when the actual request is going to be made (i.e. when the async future is spawned) and to "finalize" the builder pattern. I think it also mirrors reqwest which is nice.

I was asking because I didn't want .send() to be omitted, not because I did want it to be 😄