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

[core] remove Monkey module #322

Merged
merged 1 commit into from
Jan 24, 2018
Merged

[core] remove Monkey module #322

merged 1 commit into from
Jan 24, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Jan 22, 2018

Removing the legacy Monkey module in favor of a new API.

@delner delner added enhancement core Involves Datadog core libraries do-not-merge/WIP Not ready for merge labels Jan 22, 2018
@delner delner added this to the 0.11.1 milestone Jan 22, 2018
@delner delner self-assigned this Jan 22, 2018
@delner delner removed the do-not-merge/WIP Not ready for merge label Jan 22, 2018
Copy link
Member

@p-lambert p-lambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

Only thing worth noting here is that we're no longer testing for "idempotency", which might be a useful property of our patchers. This, however, should be now part of the unit tests of each integration. (maybe a shared example for the tests written in RSpec?)

Also worth double checking if we have a test case for Configuration#use asserting that:

Datadog.configure do |c|
  c.use :foo
end

Calls patch on :foo.

@delner
Copy link
Contributor Author

delner commented Jan 23, 2018

Shared example for RSpecs as we convert contribs over would make sense. (Means now that we have RSpec, we'd only have to code the test case once, too!)

I can look at the Configuration unit tests to see it has that example. I suspect the code itself is fine (since replacing Monkey.patch_module for configure :use allowed tests to pass), but wouldn't hurt to have the little extra coverage if we don't already.

@delner
Copy link
Contributor Author

delner commented Jan 23, 2018

@p-lambert It looks like there's already a test for Configuration#use:

  def test_use_method
      contrib = Minitest::Mock.new
      contrib.expect(:patch, true)
      contrib.expect(:sorted_options, [])

      @registry.add(:example, contrib)
      @configuration.use(:example)

      assert_mock(contrib)
    end

See

def test_use_method

Does that address all your blocking feedback?

Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @p-lambert comment is already addressed, I'm good with this change that removes a legacy API that is not used. We're losing the patch_all method (that wasn't working for some integrations), but the explicit approach is way better. We may think to reintroduce it if required in another PR.

@delner delner merged commit 8852cf9 into master Jan 24, 2018
@palazzem palazzem changed the title Kill the monkey! [core] remove Monkey module Feb 2, 2018
@delner delner deleted the delner/kill_the_monkey branch February 9, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants