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
574 stars 97 forks source link

Require "set" explicitly #311

Closed pawandubey closed 2 years ago

pawandubey commented 2 years ago

StatsD::Instrument::Expectation depends on Set being in scope, but does not explicitly require it. This works because some of its dependencies already require set:

4 matches for "\"set\"" in buffer: require
     27:requring ["set"] from /Users/pawandubey/.gem/ruby/3.1.2/gems/rubocop-1.29.1/lib/rubocop.rb:7:in `<top (required)>'
    101:requring ["set"] from /Users/pawandubey/.gem/ruby/3.1.2/gems/parser-3.1.2.0/lib/parser.rb:11:in `<top (required)>'
    158:requring ["set"] from /Users/pawandubey/.gem/ruby/3.1.2/gems/rubocop-ast-1.19.1/lib/rubocop/ast.rb:5:in `<top (required)>'
    226:requring ["set"] from /opt/rubies/3.1.2/lib/ruby/gems/3.1.0/gems/rexml-3.2.5/lib/rexml/parsers/baseparser.rb:5:in `<top (required)>'

However, for the users of statsd-instrument, this may not work as expected if they (or one of their other dependencies) are not already requiring set.

This short script, when run from the root of this project can demonstrate this:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"

  gem "statsd-instrument", path: "."
end

class FakeTest
  include StatsD::Instrument::Assertions

  def run
    expected = StatsD::Instrument::Expectation.measure("some", 1, tags: ["tag"])
    expected.matches(expected)
  end
end

FakeTest.new.run

On the master branch currently, it fails:

/Users/pawandubey/src/github.com/Shopify/statsd-instrument/lib/statsd/instrument/expectation.rb:63:in `matches': uninitialized constant StatsD::Instrument::Expectation::Set (NameError)

          expected_tags = Set.new(tags)
                          ^^^
    from require_test.rb:15:in `run'
    from require_test.rb:19:in `<main>'

While the code from this branch passes without any output, as we explicitly require set in Expectation.

pawandubey commented 2 years ago

Fascinating!

Screenshot 2022-07-14 at 9 37 41 PM