Allow ActiveSupport::Deprecation features to be used by rails applications and library authors #6348

Merged
merged 2 commits into from Sep 13, 2012

7 participants

@LTe

Updated version of #2310

@rafaelfranca
Ruby on Rails member
@jeremy
Ruby on Rails member
  1. If we're exposing this for reuse, we need docs! How should a library author use this?
  2. Feels like libraries using separate deprecators should be working with instances of a class rather than extending with a module that injects a bunch of behavior, including its internal state. Could make ActiveSupport::Deprecation a class (without breaking API) and delegate the existing toplevel methods to a singleton ActiveSupport::Deprecation.instance
  3. The #deprecator instance method may be unnecessary. Looks like the only reason we need it is so the deprecated method wrapper can refer to it. It'd be cleaner to pass a deprecator instance and reference it directly from the method wrapper (using define_method instead of class_eval)
@LTe

@jeremy done :)

@jeremy jeremy commented on an outdated diff May 18, 2012
...vesupport/lib/active_support/deprecation/behaviors.rb
- # Default warning behaviors per Rails.env.
- DEFAULT_BEHAVIORS = {
@jeremy
Ruby on Rails member
jeremy added a note May 18, 2012

Should probably leave these as a constant since it acts as public API now. People add behaviors to this hash directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Ruby on Rails member

@LTe Very nice! Looking good.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 19, 2012
...port/lib/active_support/deprecation/proxy_wrappers.rb
end
end
- class DeprecatedConstantProxy < DeprecationProxy #:nodoc:all
- def initialize(old_const, new_const)
+ # This DeprecatedConstantProxy transforms constant to depracated constant.
+ #
+ # Example
+ # OLD_CONST = ActiveSupport::Deprecation::DeprecatedConstantProxy.new('OLD_CONST', 'NEW_CONST')
+ # Example with custom deprecator
+ # OLD_CONST = ActiveSupport::Deprecation::DeprecatedConstantProxy.new('OLD_CONST', 'NEW_CONST'. deprecator_instance)
@carlosantoniodasilva
Ruby on Rails member

Typo: . instead of , after NEW_CONST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 19, 2012
...vesupport/lib/active_support/deprecation/reporting.rb
@@ -22,6 +23,13 @@ def silence
@silenced = old_silenced
end
+ # Outputs a deprecation warning message
+ # ActiveSupport::Deprecation.deprecated_method_warning(:method_name)
+ # # => "method_name is deprecated and will be removed from Rails #{deprecation_horizon}"
+ # ActiveSupport::Deprecation.deprecated_method_warning(:method_name, :another_method)
+ # # => "method_name is deprecated and will be removed from Rails #{deprecation_horizon} (use another_method instead)"
@carlosantoniodasilva
Ruby on Rails member

Indent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff May 19, 2012
activesupport/test/deprecation_test.rb
+ def message
+ ActiveSupport::Deprecation.warn 'warning in error message'
+ super
+ end
+ end
+
+ raise error_class.new('hmm')
+
+ rescue => e
+ error = Test::Unit::Error.new('testing ur doodz', e)
+ assert_not_deprecated { error.message }
+ assert_nil @last_message
+ end
+ end
+
+
@carlosantoniodasilva
Ruby on Rails member

Two lines ✂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff May 19, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
+ # "#{method_name} is deprecated and will be removed from MyLibrary | #{message}"
+ # end
+ #
+ # def warn(message, backtrace)
+ # puts(message)
+ # end
+ # end
+ # end
+ #
+ # When we deprecate method
+ # deprecate :foo => "this is very old method", :deprecator => CustomClass::Deprecator.instance
+ #
+ # It will execute
+ # CustomClass::Deprecator.instance.deprecated_method_warning(:foo, "this is very old method").tap |message|
+ # CustomClass::Deprecator.instance.warn(message, backtrace)
+ # end
@carlosantoniodasilva
Ruby on Rails member

The docs are pretty good, but there's no mention of instance in CustomClass::Deprecator, this may confuse people. Perhaps you can just use new instead of instance, or add the method or singleton there to show how it'd look like?

@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

Agree -- no need to introduce a singleton for the custom case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@LTe

@carlosantoniodasilva @jeremy thanks for feedback!

@paneq

What do you think about it:

module ActiveSupport
  class Deprecation
    def initialize(horizon = '3.2', gem_name = 'rails')
      self.deprecation_horizon = horizon
      self.gem_name = gem_name
    end
  end
end

For custom gems authors should create new instances of ActiveSupport::Deprecation to generate proper warning messages. If they use ActiveSupport::Deprecation.instance the message is going to say that something will be removed from rails which is in many cases not what the authors of gems would like to announce to the users.

These:

deprecator = ActiveSupport::Deprecation.new
deprecator.deprecation_horizon = "2.0.0"
ActiveSupport::Deprecation.new.tap{|d| d.deprecation_horizon = "2.0.0" }

seem to be unnecessary verbose to me.

ActiveSupport::Deprecation.new("2.0.0", "formtastic")

looks much better imho.

@paneq

Science Fiction:

module ActiveSupport
  class Deprecation
    def initialize(horizon = default_horizon, gem_name = 'rails')
      self.deprecation_horizon = horizon
      self.gem_name = gem_name
    end

    def default_horizon
      "#{ActiveSupport::VERSION::MAJOR}.#{ActiveSupport::VERSION::MINOR + 1}"
    end
  end
end

because sometimes it should be major + 1. Or maybe it should be always major +1 and never minor + 1 ?

@LTe

Now you can create instance of deprecator and create new object via new.
I like @paneq approach -- now this is simple to reuse.

About science fiction I think better solution is pass directly version to initialize method.

@paneq

"About science fiction I think better solution is pass directly version to initialize method." - probably. Except for the fact that the core team needs to change this one line every time there is new rails version. I wanted to spare them this trouble in this science fiction solution :)

@LTe

According to http://semver.org/ this always should be major +1

@LTe

@jeremy what do you think? We can merge current implementation?

@jeremy jeremy commented on an outdated diff May 23, 2012
...vesupport/lib/active_support/deprecation/behaviors.rb
# [+log+] Log all deprecation warnings to +Rails.logger+.
- # [+notify+] Use <tt>ActiveSupport::Notifications</tt> to notify +deprecation.rails+.
+ # [+notify] Use +ActiveSupport::Notifications+ to notify +deprecation.rails+.
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

[+notify+] formatting changed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/deprecation/method_wrappers.rb
@@ -2,25 +2,26 @@
require 'active_support/core_ext/array/extract_options'
module ActiveSupport
- module Deprecation
- # Declare that a method has been deprecated.
- def self.deprecate_methods(target_module, *method_names)
- options = method_names.extract_options!
- method_names += options.keys
+ class Deprecation
+ module MethodWrapper
+ # Declare that a method has been deprecated.
+ def deprecate_methods(target_module, *method_names)
+ options = method_names.extract_options!
+ deprecator = options.delete(:deprecator) || ActiveSupport::Deprecation
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

ActiveSupport::Deprecation.instance here rather than rely on the instance delegator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...port/lib/active_support/deprecation/proxy_wrappers.rb
@@ -25,10 +25,21 @@ def method_missing(called, *args, &block)
end
end
- class DeprecatedObjectProxy < DeprecationProxy #:nodoc:
- def initialize(object, message)
+ # This DeprecatedObjectProxy transforms object to depracated object.
+ #
+ # Example
+ # @old_object = DeprecatedObjectProxy.new(Object.new, "Don't use this object anymore!")
+ # Example with custom deprecator
+ # @old_object = DeprecatedObjectProxy.new(Object.new, "Don't use this object anymore!", deprecator_instance)
+ #
+ # When someone execute any method expect +inspect+ on proxy object this will trigger +warn+ method on +deprecator_instance+
+ #
+ # Default deprecator is ActiveSupport::Deprecation
+ class DeprecatedObjectProxy < DeprecationProxy
+ def initialize(object, message, deprecator = ActiveSupport::Deprecation)
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

.instance here too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@LTe

@jeremy updated and rebased

@jeremy jeremy commented on an outdated diff May 23, 2012
...vesupport/lib/active_support/deprecation/behaviors.rb
@@ -1,8 +1,32 @@
require "active_support/notifications"
module ActiveSupport
- module Deprecation
- class << self
+ class Deprecation
+ # Default warning behaviors per Rails.env.
+ DEFAULT_BEHAVIORS = {
+ :stderr => Proc.new { |message, callstack|
+ $stderr.puts(message)
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

Extra indentation here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on the diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
@@ -5,6 +5,36 @@ class Module
# deprecate :foo
# deprecate :bar => 'message'
# deprecate :foo, :bar, :baz => 'warning!', :qux => 'gone!'
+ #
+ # You can use custom deprecator instance
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

You can use a custom deprecator object that responds to +warn+ and +deprecated_method_warning+. This is useful when you use +ActiveSupport::Deprecation+ in your own libraries and want to customize deprecation behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
# deprecate :foo
# deprecate :bar => 'message'
# deprecate :foo, :bar, :baz => 'warning!', :qux => 'gone!'
+ #
+ # You can use custom deprecator instance
+ # deprecate :foo, :deprecator => CustomClass::Deprecator.new
+ # deprecate :foo, :bar => "warning!", :deprecator => CustomClass::Deprecator.new
+ #
+ # Custom deprecator instance need respond to two methods
+ # [deprecated_method_warning(method_name, message)] Method will be executed with *deprecated* method name
+ # [warn(message, backtrace)] Method will accept message from *deprecated_method_warning* method and backtrace
+ #
+ # Example
+ # class CustomClass
+ # class Deprecator
+ # def deprecated_method_warning(method_name, message)
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012
class MyLib::Deprecator
  def warn(message, backtrace)
    Kernel.warn message
  end

  def deprecated_method_warning(method_name, message)
    "#{method_name} is deprecated and will be removed from MyLibrary | #{message}"
  end
end
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012
module MyLib
  mattr_accessor :deprecator
  self.deprecator = Deprecator.new
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
+ #
+ # You can use custom deprecator instance
+ # deprecate :foo, :deprecator => CustomClass::Deprecator.new
+ # deprecate :foo, :bar => "warning!", :deprecator => CustomClass::Deprecator.new
+ #
+ # Custom deprecator instance need respond to two methods
+ # [deprecated_method_warning(method_name, message)] Method will be executed with *deprecated* method name
+ # [warn(message, backtrace)] Method will accept message from *deprecated_method_warning* method and backtrace
+ #
+ # Example
+ # class CustomClass
+ # class Deprecator
+ # def deprecated_method_warning(method_name, message)
+ # "#{method_name} is deprecated and will be removed from MyLibrary | #{message}"
+ # end
+ #
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012
class MyLib::Bar
  deprecate :foo => "this is very old method", :deprecator => MyLib.deprecator
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
+ # [deprecated_method_warning(method_name, message)] Method will be executed with *deprecated* method name
+ # [warn(message, backtrace)] Method will accept message from *deprecated_method_warning* method and backtrace
+ #
+ # Example
+ # class CustomClass
+ # class Deprecator
+ # def deprecated_method_warning(method_name, message)
+ # "#{method_name} is deprecated and will be removed from MyLibrary | #{message}"
+ # end
+ #
+ # def warn(message, backtrace)
+ # puts(message)
+ # end
+ # end
+ # end
+ #
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

It will build a deprecation warning message by calling MyLib.deprecator.deprecated_method_warning(:foo, "this is a very old method") then invoke the deprecator warning with the message and the caller's backtrace: MyLib.deprecator.warn(warning_message, caller)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
@@ -1,10 +1,43 @@
require 'active_support/deprecation/method_wrappers'
class Module
- # Declare that a method has been deprecated.
+ # You can use a custom deprecator object that responds to +warn+ and +deprecated_method_warning+.
+ # This is useful when you use +ActiveSupport::Deprecation+ in your own libraries and want to customize
+ # deprecation behavior.
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

Can leave these out, they're better demonstrated below

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
@@ -1,10 +1,43 @@
require 'active_support/deprecation/method_wrappers'
class Module
- # Declare that a method has been deprecated.
+ # You can use a custom deprecator object that responds to +warn+ and +deprecated_method_warning+.
+ # This is useful when you use +ActiveSupport::Deprecation+ in your own libraries and want to customize
+ # deprecation behavior.
+ #
# deprecate :foo
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

Custom deprecators must respond to two methods:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
@@ -1,10 +1,43 @@
require 'active_support/deprecation/method_wrappers'
class Module
- # Declare that a method has been deprecated.
+ # You can use a custom deprecator object that responds to +warn+ and +deprecated_method_warning+.
+ # This is useful when you use +ActiveSupport::Deprecation+ in your own libraries and want to customize
+ # deprecation behavior.
+ #
# deprecate :foo
# deprecate :bar => 'message'
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

[deprecated_method_warning(method_name, message)] will be called with the deprecated method name and the message it was declared with. Return a warning message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff May 23, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
@@ -1,10 +1,43 @@
require 'active_support/deprecation/method_wrappers'
class Module
- # Declare that a method has been deprecated.
+ # You can use a custom deprecator object that responds to +warn+ and +deprecated_method_warning+.
+ # This is useful when you use +ActiveSupport::Deprecation+ in your own libraries and want to customize
+ # deprecation behavior.
+ #
# deprecate :foo
# deprecate :bar => 'message'
# deprecate :foo, :bar, :baz => 'warning!', :qux => 'gone!'
@jeremy
Ruby on Rails member
jeremy added a note May 23, 2012

[warn(message, backtrace)] will be called with the message from your deprecated_method_warning method and the caller's backtrace. Implement whatever warning behavior you like here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy
Ruby on Rails member

@LTe some comments on the documentation :)

It covers how to give a custom deprecator object, but doesn't show how you can use ActiveSupport::Deprecation.new(version, libname) yourself.

When I read through the duck type for a deprecator, having to implement two methods seems wrong, too. We call deprecator.deprecated_method_warning then pass its return value to #warn -- we could just call one method and let the deprecator handle that internally.

@paneq

@jeremy - I think that's because #warn has the logic for outputing to logger, stderr, output or whatever is configured in current development environment and #deprecated_method_warning is just responsible for generting the warning. One might want to generate different warnings for his own gem but keep the logic coherent with Rails as to where they are displayed. What do you think ?

@jeremy
Ruby on Rails member

@paneq definitely -- and a higher-level method like deprecation_warning(deprecated_method_name, message, caller_backtrace) could wrap that up. Then custom classes can implement that one method whereas subclasses of ActiveSupport::Deprecation can override just deprecated_method_warning if they like.

@LTe
LTe commented Jun 7, 2012

Updated

@LTe

@jeremy what do you think about merge?

@steveklabnik
Ruby on Rails member

@LTe it needs rebased at least, it can't be merged cleanly anymore.

@LTe
LTe commented Jul 2, 2012

@steveklabnik rebased ;-)

@steveklabnik
Ruby on Rails member

Thanks. Let's see what @jeremy says.

@paneq

What do you think @jeremy ?

@jeremy
Ruby on Rails member

👍

@carlosantoniodasilva carlosantoniodasilva commented on the diff Sep 11, 2012
...ort/lib/active_support/core_ext/module/deprecation.rb
+ #
+ # \Custom deprecators must respond to one method
+ # [deprecation_warning(deprecated_method_name, message, caller_backtrace)] will be called with the deprecated
+ # method name, the message it was declared
+ # with and caller_backtrace. Implement
+ # whatever warning behavior you like here.
+ #
+ # Example
+ # class MyLib::Deprecator
+ #
+ # def deprecation_warning(deprecated_method_name, message, caller_backtrace)
+ # message = "#{method_name} is deprecated and will be removed from MyLibrary | #{message}"
+ # Kernel.warn message
+ # end
+ #
+ # end
@carlosantoniodasilva
Ruby on Rails member

Please remove whitespaces between the method and class declaration, there's no need for them ✂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Sep 11, 2012
activesupport/test/deprecation_test.rb
+ deprecator = ActiveSupport::Deprecation.new
+
+ deprecator.send(:deprecated_method_warning, :deprecated_method, "You are calling deprecated method").tap do |message|
+ assert_match(/is deprecated and will be removed from Rails/, message)
+ end
+ end
+
+ def test_custom_gem_name
+ deprecator = ActiveSupport::Deprecation.new('2.0', 'Custom')
+
+ deprecator.send(:deprecated_method_warning, :deprecated_method, "You are calling deprecated method").tap do |message|
+ assert_match(/is deprecated and will be removed from Custom/, message)
+ end
+ end
+
+ unless defined?(::MiniTest)
@carlosantoniodasilva
Ruby on Rails member

I think this will be always defined, why do you need to test using `Test::Unit::Error in this particular case?

@steveklabnik
Ruby on Rails member

This will be, except on some distros like Fedora which (imho) give you a broken Ruby.

Especially on master, where we only support 1.9, this should basically always be true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

@LTe hey, could you please add a changelog entry for this, and check the minor comments I made, so that we can get this in master? Thanks!

@LTe

@carlosantoniodasilva updated, rebased.

@carlosantoniodasilva
Ruby on Rails member

@LTe, great thanks. I'll have to ask you one more thing though, to review your commit message and improve the changelog a bit with an example of how to use this new feature. This will help others to understand the reasoning and how to use the feature at the same time when reading the commits or the changelog.

Here's an explanation about how to go with the commit message, and here's another about changelogs.

I'll merge it afterwards. Thanks!

paneq and others added some commits Jul 25, 2011
@paneq paneq extend ActiveSupport::Deprecation with self, allow other objects to e…
…xtend/include it also.

test local deprecation

deprecator object

Test ActiveSupport::Deprecation when included
2c690a0
@LTe LTe Change ActiveSupport::Deprecation to class.
ActiveSupport::Deprecation is now a class rather than a module. You can
get instance of ActiveSupport::Deprecation calling #instance method.

  ActiveSupport::Deprecation.instance

But when you need to get new object od ActiveSupport::Deprecation you
need to just call #new.

  @instance = ActiveSupport::Deprecation.new

Since you can create a new object, you can change the version and the
name of the library where the deprecator concerned.

  ActiveSupport::Deprecation.new('2.0', 'MyGem')

If you need use another deprecator instance you can select it in the
options of deprecate method.

  deprecate :method, :deprecator => deprecator_instance

Documentation has been updated.
71993c6
@carlosantoniodasilva
Ruby on Rails member

@LTe great, thank you!

@carlosantoniodasilva carlosantoniodasilva merged commit 01ef633 into rails:master Sep 13, 2012
@splattael

The definition of self.deprecator should go before this deprecate call.

Ruby on Rails member

Fixed @ 8692db5

Ruby on Rails member

<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment