Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(metrics/histogram): 🎂 constructor accepts IntoIterator #243

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cratelyn
Copy link
Contributor

this is a very small, non-breaking, alteration to the signature of Histogram's constructor.

rather than accepting an impl Iterator, this commit changes the parameter of Histogram::new() to be an impl IntoIterator instead. this does not affect the existing contract because of the blanket impl<I: Iterator> IntoIterator for I implementation provided by the standard library.

by accepting IntoIterator however, callers providing a collection such as a [f64; 5] array or a Vec<f64> vector do not need to invoke into_iter() themselves at the call-site.

// now, constructing a histogram needn't involve `into_iter()`...
use prometheus_client::metrics::histogram::Histogram;
let histogram = Histogram::new([10.0, 100.0, 1_000.0]);

this leans on the same sugar used by for {} loops, see the relevant section of the std::iter documentation here:
https://doc.rust-lang.org/stable/std/iter/index.html#for-loops-and-intoiterator

no changes are needed within Histogram::new() because we already call into_iter() on the provider iterator when appending f64::MAX and collecting the buckets into a Vec<_>.

@cratelyn cratelyn changed the title refactor(metrics/histogram): constructor accepts IntoIterator refactor(metrics/histogram): 🎂 constructor accepts IntoIterator Nov 18, 2024
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Neat!

@cratelyn would you mind adding a changelog entry?

this is a very small, **non-breaking**, alteration to the signature of
`Histogram`'s constructor.

rather than accepting an `impl Iterator`, this commit changes the
parameter of `Histogram::new()` to be an `impl IntoIterator` instead.
this does not affect the existing contract because of the blanket
`impl<I: Iterator> IntoIterator for I` implementation provided by the
standard library.

by accepting `IntoIterator` however, callers providing a collection such
as a `[f64; 5]` array or a `Vec<f64>` vector do not need to invoke
`into_iter()` themselves at the call-site.

```rust
// now, constructing a histogram needn't involve `into_iter()`...
use prometheus_client::metrics::histogram::Histogram;
let histogram = Histogram::new([10.0, 100.0, 1_000.0]);
```

this leans on the same sugar used by `for {}` loops, see the relevant
section of the `std::iter` documentation here:
<https://doc.rust-lang.org/stable/std/iter/index.html#for-loops-and-intoiterator>

no changes are needed within `Histogram::new()` because we already call
`into_iter()` on the provider iterator when appending `f64::MAX` and
collecting the buckets into a `Vec<_>`.

Signed-off-by: katelyn martin <[email protected]>
@cratelyn cratelyn force-pushed the histogram-into-iter branch from 9993c64 to 1e216f1 Compare January 8, 2025 19:55
@cratelyn
Copy link
Contributor Author

cratelyn commented Jan 8, 2025

sure thing! i've rebased this on the latest master branch, and have added a changelog entry for this pull request. how does 1e216f1 look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants