stats: add config to disable count per bucket #52
Conversation
AlexandreYang
force-pushed
the
alex/vendasta/disable-count-per-bucket
branch
from
194eaaf
to
b02c899
Compare
gbbr
changed the title
AlexandreYang
force-pushed
the
alex/vendasta/disable-count-per-bucket
branch
from
d0a224b
to
78d78bd
Compare
AlexandreYang
force-pushed
the
alex/vendasta/disable-count-per-bucket
branch
from
78d78bd
to
cd42213
Compare
AlexandreYang
changed the title
AlexandreYang
changed the title
There was a problem hiding this comment.
Thanks Alexandre! Unfortunately you have still not addressed all comments. What I recommend doing is to take each comment one by one, address it, and after you do so, reply to them and mark them as resolved.
If you don't agree with some of the comments, that's also ok, but please reply on each one of them.
|
Thanks for the quick feedback. Just made another update. Let me know if that's ok for you. |
There was a problem hiding this comment.
Thanks a lot for your patience! Left one more comment.
Also, I'd suggest not resolving a comment on which we haven't yet reached a consensus (common ground). When a comment is resolved it generally means that we both agreed on the outcome, whereas some comments (like the test renaming), are still open for discussion.
stats_test.go
Outdated
| } | ||
| }) | ||
|
|
||
| t.Run("count per buckets disabled", func(t *testing.T) { |
There was a problem hiding this comment.
For the same reason as stated in #52 (comment), the general pattern is to use one or two words max. I think in this case it is obvious that one tests verifies the feature and the other verifies that it can be disabled. So I suggest, either:
onandoffenabledordisabledokandoffokordisabled
I think it is clear enough. Adding sentences and more words makes the tests look ugly when running.
There was a problem hiding this comment.
Sure. Make sense. Replaced by disabled.
I also refactored to use a map of testCases to reduce duplication. See my last commit.
AlexandreYang
force-pushed
the
alex/vendasta/disable-count-per-bucket
branch
from
c081f42
to
3a2bdb3
Compare
Description
Add config to disable count per bucket
Motivation
Initial request:
#41