Skip to content

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