search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

Change Behavior of Get function on Prefixed Registry to be more consi… #175

Closed drichelson closed 7 years ago

drichelson commented 8 years ago

…stent. Update tests. Addresses https://github.com/rcrowley/go-metrics/issues/174

mihasya commented 8 years ago

The change is correct, but I do have to ask why the usage of NewCounter etc had to be changed to add () - was something about this incorrect before? I don't know the code here as well, so I'm not sure what the expectations are.

drichelson commented 8 years ago

@mihasya Adding the ()s was not necessary for tests to pass. I added it to test functions that did not have 'Lazy' in their names since those are testing lazy registration, and it seemed to me that without the lazy name we ought to just test passing in a metric unlazily.

If we're intending to test something about laziness in those functions I can revert my ()s and rename them.

mihasya commented 8 years ago

Sorry missed the last comment.

I had asked about the parentheses because there just seems to be a bunch of changes in the PR that have nothing to do with the actual change you're proposing. This makes the review harder, because I have to process the extraneous test changes - is the test still checking the same thing? If not, is the test being adjusted to accommodate behavior that's changing? It's also all in one commit, which muddies intent more.As you can tell, I don't get to look at these PRs every day (or even every week), so it's a lot of context to build up. With only the "new" tests, I would have merged that PR the same day it was made.

Anyway, I looked over the changes, and everything appears correct. In the future, please keep changes in PRs scoped to just the proposed change to expedite review. As always, thank you for taking the time to fix the bug, and I apologize for the lengthy review.