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

feat: automatically convert Vec into HashMap in histogram_opts! macro #422

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dkryptr
Copy link

@dkryptr dkryptr commented Oct 21, 2021

I noticed that this happens in the opts! macro already so I think it makes sense for histogram_opts! to also do this.

@dkryptr dkryptr changed the title feat: automatically convert Vec into HashMap feat: automatically convert Vec into HashMap in histogram_opts! macro Oct 21, 2021
@lucab
Copy link
Member

lucab commented Oct 25, 2021

Thanks for the PR! CI does not seem to like the proposed change, but I didn't look deeper into what's going on. On which toolchain version are you developing this? Does cargo test succeed for you locally?

@dkryptr
Copy link
Author

dkryptr commented Oct 25, 2021

Whoops, I had made the original commit directly from Github without cloning and testing. I've updated the tests and everything is working.

This is a breaking change to existing functionality since histogram_opts! now expects labels to be passed in like labels! {"key" => "value"} instead of labels! {"key".to_string() => "value".to_string()}. This is how opts! works so I think it's an acceptable breaking change.

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