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

Add Clear to Meter and Timer #113

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

Conversation

henning77
Copy link

I needed Clear() on both Meter and Timer to reset these after a unit test. Counter, Histogram, Sample already have Clear so I added equivalent methods.

@freeformz
Copy link

+1 Can we get this merged?

@FZambia
Copy link

FZambia commented Jul 4, 2015

+1 as this should resolve #120

@mihasya
Copy link
Collaborator

mihasya commented Jul 4, 2015

(starting to try and catch up on the backlog)

I am reluctant to accept a change which extends an interface, then immediately proceeds to add implementations which panic to half of the implementations.

Could the method just be added to the implementation you're testing and left out of the interface?

@mihasya
Copy link
Collaborator

mihasya commented Jul 4, 2015

@FZambia I'll comment in the ticket itself, but I wanted to make it clear here too: nothing you've described in #120 sounds like a bug to me.

@mihasya
Copy link
Collaborator

mihasya commented Nov 29, 2015

Bump, @henning77 can you weigh in on the question from July 4th? Thanks!

@henning77
Copy link
Author

@mihasya Clear() panics when called on the snapshot. Which is reasonable and in line with the other Clear() implementations, e.g. in https://github.com/rcrowley/go-metrics/blob/master/histogram.go

@yalay
Copy link

yalay commented Sep 2, 2016

+1 Can we get this merged? Seems to be reasonable.

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.

5 participants