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

Remove unnecessary panic #40

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

Remove unnecessary panic #40

wants to merge 1 commit into from

Conversation

rkuska
Copy link

@rkuska rkuska commented Feb 19, 2020

Hey there, hope you don't mind a PR. I was checking the code as I am considering using it in my project and I noticed a panic in it. I therefore decided to remove it and it seems like this change should keep the functionality as it was before - but changing the function signature. It would require a bump in major version of the module. Feel free to close this if you think it is too intrusive. Commit message follows.

This is a breaking change as it changes the signature of prometheus
factory but it removes unnecessary panic in the code. Previously the
function changed was defined with variadic parameter for additional
list of metrics just to make the additional list optional. This can be
done also by using a variadic parameter where each metric is passed on
its own. This should be also preferred as user is not forced to create a
slice when passing a parameter.
We also use the existing lengths to pre-calculate the capacity of the
final list of metrics.

This is a breaking change as it changes the signature of prometheus
factory but it removes unnecessary panic in the code. Previously the
function changed was defined with variadic parameter for additional
list of metrics just to make the additional list optional. This can be
done also by using a variadic parameter where each metric is passed on
its own. This should be also preferred as user is not forced to create a
slice when passing a parameter.
We also use the existing lengths to pre-calculate the capacity of the
final list of metrics.
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.

1 participant