Shopify / deprecation_toolkit

⚒Eliminate deprecations from your codebase ⚒
MIT License
460 stars 40 forks source link

Add DeprecationToolkit.initialize method #25

Closed rmacklin closed 5 years ago

rmacklin commented 5 years ago

This method is, by default, called by the Minitest plugin init hook: https://github.com/Shopify/deprecation_toolkit/blob/802a0b8dd525bc0e9c4142127cba51dc0ffadd76/lib/minitest/deprecation_toolkit_plugin.rb#L12-L18

However, it can be called earlier if you want to catch deprecations that are emitted before Minitest initializes its plugins (which is at_exit).

Addresses https://github.com/Shopify/deprecation_toolkit/issues/20

rmacklin commented 5 years ago

This is not perfect.

Basically, users could opt to call DeprecationToolkit.initialize at the beginning of their test_helper, which is earlier than when Minitest would initialize the plugin.

Then, if any deprecations that were emitted before the first test is run, that first test will fail.

So, it's useful with the Raise behavior, but it doesn't work with the Record behavior since the first test that runs is random (unless you're not running in randomized order).

But since there is no need to call the initialize method in test_helper yourself, maybe we can settle for this?

Edouard-chin commented 5 years ago

Thanks for opening this PR. I'm not convinced about this solution especially since it doesn't work with the RecordBehaviour. I'd rather have something that works out of the box and don't require the application to setup something.

rmacklin commented 5 years ago

That makes sense. I agree this isn't an ideal solution to #20. However, I do think these changes offer an improvement over what the gem provides today (which is no ability to catch deprecations that are logged outside the test cases themselves).

Would you consider merging this PR to add the initialize method, but leaving the issue open? The gem would still work out of the box without the application setting something up the same way it does today, because the minitest plugin calls initialize for you. And by leaving the issue open, we'd signal that we're still looking for a solution for that problem (separate from adding an initialize method, which basically allows for a workaround, not a robust solution).

Edouard-chin commented 5 years ago

I didn't want to add the initialize method because it adds unnecessary indirection only to workaround an edge case issue. In https://github.com/Shopify/deprecation_toolkit/pull/26 I made a change that allows you to call DeprecationToolkit.add_notify_behavior / DeprecationToolkit.attach_subscriber whenever you want and when the minitest plugin kicks in it won't redo it again.

I know it's not the real fix of the issue but still something that needed to be fixed anyway and should allow you to catch class level deprecations.