Closed xitep closed 8 years ago
@xitep I'm following the same patterns as used in mio. See: https://github.com/carllerche/mio/blob/master/src/event_loop.rs
would you still be fine with the .with_
methods? it's fine if you're not, feel free to close this issue then. otherwise i'd submit a PR.
I'm not seeing the benefit to the additional .with_
methods. Constructing the HistogramConfig
and then passing to the ::configured()
method seems to keep in-line with other libraries out there.
ok, thanks anyway.
just to give you an idea of the benefit (i see with the proposal); currently i need to do:
let mut hist = {
let mut cfg = HistogramConfig::new();
cfg.max_value(10_000);
.precision(3);
Histogram::configured(cfg).unwrap()
};
which is fully ok; it's just more verbose (and less fluent) than the proposal.
You can chain the cfg.max_value().precision()
. But it is still 3 lines to get a configured Histogram. Are there libraries you're aware of that implement better ergonomics here?
Well, a prominent example might be the command API in the stdlib; the trick that is employed here (and could be employed in histogram
as well) is that the "builder" has a "create" methods, i.e. .output()
(so this avoids the necessity to store the builder in a local variable.)
Having this (in the histogram lib):
impl HistogramConfig {
...
pub fn build(&self) -> Option<Histogram> {
Histogram::configured(*self)
}
...
seems to do the trick :)
// client code
use histogram::HistogramConfig;
let mut hist = HistogramConfig::new()
.max_value(100)
.precision(1)
.build()
.unwrap();
hist.increment(1);
I don't like the naming (in this latter example) but i like the fact that i don't worry about a variable name for the configuration struct :)
Another example where ergonomics have been a consideration is here: https://spicavigo.github.io/kafka-rust/kafka/consumer/index.html (a lib i'm working on occasionally)
Thanks for the examples. Re-opened while I ponder this further.
What if we offer something like
let mut h = Histogram::configure()
.max_value(10_000)
.precision(3)
.build()
.unwrap();
This looks wonderful to me!
thank you!
Would there be any argument against having
HistogramConfig
's methods returnSelf
instead of&mut Self
? This would allow a very nice:Alternatively, I could imagine the following additional methods (while keeping the old ones as their are):
If one of these would be ok from your side, I could submit a PR.