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

src/macros: Have labels! accept any type that implements Display #350

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

Conversation

free
Copy link
Contributor

@free free commented Oct 3, 2020

This allows (among other things) histogram_opts! to take the same kinds of
types as opts!. E.g. to replace this:

let status = String::from("200");
histogram_opts!(
    ...
    labels! {"status".to_string() => status.clone()}
)
// Do more stuff with `status`.

with this:

let status = String::from("200");
histogram_opts!(
    ...
    labels! {"status" => &status}
)
// Do more stuff with `status`.

And a lot more, such as using numbers or enums as label values directly.

Signed-off-by: Alin Sinpalean [email protected]

This allows (among other things) `histogram_opts!` to take the same kinds of
types as `opts!`. E.g. to replace this:

```rust
let status = String::from("200");
histogram_opts!(
    ...
    labels! {"status".to_string() => status.clone()}
)
// Do more stuff with `status`.
```

with this:

```rust
let status = String::from("200");
histogram_opts!(
    ...
    labels! {"status" => &status}
)
// Do more stuff with `status`.
```

And a lot more, such as using numbers or enums as label values directly.

Signed-off-by: Alin Sinpalean <[email protected]>
@free free force-pushed the macros-labels-accept-Display-types branch from af94a02 to a722af4 Compare October 3, 2020 10:22
@breezewish
Copy link
Member

I'm not sure whether this is a good idea.. @BusyJay PTAL

@BusyJay
Copy link
Member

BusyJay commented Oct 9, 2020

I don't think it's a good idea to introduce avoidable clone behind a library interface. Current way allow callers to decide whether to create or reuse an owned string.

@free
Copy link
Contributor Author

free commented Oct 9, 2020

I fully understand if this is not accepted as is, but I'd like to point out that AFAICT no one is going to be creating thousands upon thousands of metrics, so the cost is essentially constant.

The alternative would be to use into() instead. Or (likely by using a helper trait) try into()if the type implements Into<String> and fall back to ToString otherwise. I don't think it's worth the trouble, but I can give it a shot if you want.

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.

3 participants