elastic / elasticsearch-rs

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

[DISCUSS] Builder associated functions accepting Option<T> #17

Closed russcam closed 4 years ago

russcam commented 4 years ago

Each builder struct models all fields as Option<T>. Fields include all query string parameters and the body of the request, if applicable. The associated functions to assign values to fields on the builder struct all accept Option<T> too. For example

let mut response = client
    .search(SearchUrlParts::None)
    .pretty(Some(true))
    .q(Some("title:Elasticsearch".into()))
    .send()?;

This issue is to discuss whether the associated functions should accept Option<T> or simply T in each case. The above example would then become

let mut response = client
    .search(SearchUrlParts::None)
    .pretty(true)
    .q("title:Elasticsearch".into())
    .send()?;

One advantage of accepting Option<T> is that it is possible to clone a builder and pass None for those values that one does not wish to send on the clone. If functions only accept T this would not be possible, although it can be argued how often are consumers likely to want to clone and None out values? One disadvantage of accepting Option<T> everywhere is that it makes using the client more verbose because each value needs to be passed as Some(T).

Interested to hear relative merits of each approach.

russcam commented 4 years ago

I've opened https://github.com/elastic/elasticsearch-rs/pull/23 to show what this looks like. It's a very small change in the api_generator for this.