-
Notifications
You must be signed in to change notification settings - Fork 375
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
Conversation
5a38c6a
to
a3ba36f
Compare
There was a problem hiding this 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
.
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 |
@p-lambert It looks like there's already a test for 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 dd-trace-rb/test/configuration_test.rb Line 12 in a3ba36f
Does that address all your blocking feedback? |
There was a problem hiding this 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.
Removing the legacy
Monkey
module in favor of a new API.