Shopify / deprecation_toolkit

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

DeprecationToolkit patches Kernel#warn but not Kernel.warn #19

Closed rmacklin closed 5 years ago

rmacklin commented 5 years ago

Here is a failing test that demonstrates this:

diff --git a/test/deprecation_toolkit/warning_test.rb b/test/deprecation_toolkit/warning_test.rb
index 0c519cc..c3f9e69 100644
--- a/test/deprecation_toolkit/warning_test.rb
+++ b/test/deprecation_toolkit/warning_test.rb
@@ -22,6 +22,16 @@ module DeprecationToolkit
       end
     end

+    test 'treats warnings as deprecations using Kernel.warn' do
+      Configuration.warnings_treated_as_deprecation = [/#example is deprecated/]
+
+      assert_raises Behaviors::DeprecationIntroduced do
+        Kernel.warn '#example is deprecated'
+
+        trigger_deprecation_toolkit_behavior
+      end
+    end
+
     test 'warn can be called with an array' do
       Configuration.warnings_treated_as_deprecation = [/#example is deprecated/, /#something is deprecated/]

Kernel.warn is used in place of Kernel#warn in places where warn would not resolve to Kernel#warn, e.g. here.

rmacklin commented 5 years ago

I can open a PR, but wanted to get thoughts from the maintainers about how they would like to see it implemented.

Here's the current code:

if RUBY_VERSION < '2.5.0' && RUBY_ENGINE == 'ruby'
  module Kernel
    alias_method :__original_warn, :warn

    def warn(*messages)
      message = messages.join("\n")
      message += "\n" unless message.end_with?("\n")

      if DeprecationToolkit::Warning.deprecation_triggered?(message)
        ActiveSupport::Deprecation.warn(message)
      else
        __original_warn(messages)
      end
    end
  end
end

The simplest solution would be to duplicate the patch inside class << self:

if RUBY_VERSION < '2.5.0' && RUBY_ENGINE == 'ruby'
  module Kernel
    class << self
      alias_method :__original_warn, :warn

      def warn(*messages)
        message = messages.join("\n")
        message += "\n" unless message.end_with?("\n")

        if DeprecationToolkit::Warning.deprecation_triggered?(message)
          ActiveSupport::Deprecation.warn(message)
        else
          __original_warn(messages)
        end
      end
    end

    alias_method :__original_warn, :warn

    def warn(*messages)
      message = messages.join("\n")
      message += "\n" unless message.end_with?("\n")

      if DeprecationToolkit::Warning.deprecation_triggered?(message)
        ActiveSupport::Deprecation.warn(message)
      else
        __original_warn(messages)
      end
    end
  end
end

but please share your preferred implementation.

Edouard-chin commented 5 years ago

Thanks a lot for opening the issue. I went ahead and implemented a fix based on your snippet. I credited you in the Changelog