Rubocop: run our own cops on this repo.

Shopify / statsd-instrument

A StatsD client for Ruby apps. Provides metaprogramming methods to inject StatsD instrumentation into your code.

http://shopify.github.io/statsd-instrument

MIT License

570 stars 94 forks source link

Rubocop: run our own cops on this repo. #181

Closed wvanbergen closed 5 years ago

wvanbergen commented 5 years ago

We have some nice Rubocop rules. We should make sure our codebase conforms. By doing so, I already found some interesting things:

StatsD.key_value's third argument is secretly actually a timestamp, not a sample rate. So changing this to sample_rate: timestamp does not make sense. We probably should figure out how we want to handle this. The README doesn't mention this at all (https://github.com/statsite/statsite), so maybe we should simply :fire: this?
Our metaprogramming methods (stats_count and friends) still rely on positional arguments. I am pretty sure I also found a bug (a missing value argument). How do we want to deal with this? I am not a big fan of these methods, but we cannot really get rid of them.

dylanahsmith commented 5 years ago

StatsD.key_value's third argument is secretly actually a timestamp, not a sample rate.

How did you find out about that? Is the timestamp an expiration time? If we want to support that, then we should use an appropriately named keyword argument.

Our metaprogramming methods (stats_count and friends) still rely on positional arguments.

You mean the usage of them in the application? Since it seems like they just pass options with a *metric_options splat which supports both keyword and positional arguments.

wvanbergen commented 5 years ago

You mean the usage of them in the application? Since it seems like they just pass options with a *metric_options splat which supports both keyword and positional arguments

This works fine (except in the case where we have the bug), but it doesn't satisfy our Rubocop rule about positional arguments.

Now I could also change the Rubocop rule, but I decided against it. After StatsD.metric(name, value, ... we should expect only keyword arguments. So I think it is correct for the cop to tell people to change it to **metric_options instead. The rule will not auto-correct this though.

Either way, I fixed this issue in #182.

wvanbergen commented 5 years ago

Anyway, all the issues I brought up should be fixed in separate PRs - the focus of this PR is simply to get Rubocop to run with our own rules on our repo.