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

LTe · 2012-05-16T09:40:54Z

Updated version of #2310

rafaelfranca · 2012-05-16T13:17:38Z

cc @josevalim @jeremy

jeremy · 2012-05-16T14:11:55Z

If we're exposing this for reuse, we need docs! How should a library author use this?
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
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 · 2012-05-18T12:05:38Z

@jeremy done :)

jeremy · 2012-05-18T21:18:56Z

activesupport/lib/active_support/deprecation/behaviors.rb


-    # Default warning behaviors per Rails.env.
-    DEFAULT_BEHAVIORS = {


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

jeremy · 2012-05-18T21:22:10Z

@LTe Very nice! Looking good.

carlosantoniodasilva · 2012-05-19T13:40:56Z

activesupport/lib/active_support/deprecation/proxy_wrappers.rb

+    # 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)


Typo: . instead of , after NEW_CONST.

LTe · 2012-05-19T17:52:05Z

@carlosantoniodasilva @jeremy thanks for feedback!

paneq · 2012-05-20T22:28:53Z

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 · 2012-05-20T22:31:03Z

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 · 2012-05-21T10:03:37Z

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 · 2012-05-21T10:24:24Z

"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 · 2012-05-21T10:30:03Z

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

LTe · 2012-05-23T10:06:31Z

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

jeremy · 2012-05-23T12:39:30Z

activesupport/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+.


[+notify+] formatting changed?

LTe · 2012-05-23T12:58:21Z

@jeremy updated and rebased

jeremy · 2012-05-23T13:55:58Z

activesupport/lib/active_support/deprecation/behaviors.rb

+    # Default warning behaviors per Rails.env.
+    DEFAULT_BEHAVIORS = {
+        :stderr => Proc.new { |message, callstack|
+          $stderr.puts(message)


Extra indentation here

jeremy · 2012-05-23T14:19:25Z

@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 · 2012-05-23T14:23:03Z

@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 · 2012-05-23T14:48:19Z

@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 · 2012-06-07T09:56:48Z

Updated

LTe · 2012-06-29T12:18:39Z

@jeremy what do you think about merge?

steveklabnik · 2012-07-01T16:21:53Z

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

LTe · 2012-07-02T07:30:21Z

@steveklabnik rebased ;-)

steveklabnik · 2012-07-02T14:23:41Z

Thanks. Let's see what @jeremy says.

paneq · 2012-07-09T09:46:57Z

What do you think @jeremy ?

jeremy · 2012-09-11T18:49:27Z

👍

carlosantoniodasilva · 2012-09-11T18:51:03Z

activesupport/lib/active_support/core_ext/module/deprecation.rb

+  #         Kernel.warn message
+  #      end
+  #
+  #    end


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

carlosantoniodasilva · 2012-09-11T18:57:13Z

@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 · 2012-09-12T16:15:35Z

@carlosantoniodasilva updated, rebased.

carlosantoniodasilva · 2012-09-12T19:58:20Z

@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!

…xtend/include it also. test local deprecation deprecator object Test ActiveSupport::Deprecation when included

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.

LTe · 2012-09-13T06:42:28Z

@carlosantoniodasilva updated

carlosantoniodasilva · 2012-09-13T11:46:58Z

@LTe great, thank you!

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

jeremy reviewed May 18, 2012
View reviewed changes

carlosantoniodasilva reviewed May 19, 2012
View reviewed changes

jeremy reviewed May 23, 2012
View reviewed changes

carlosantoniodasilva reviewed Sep 11, 2012
View reviewed changes

paneq and others added 2 commits September 13, 2012 08:42

extend ActiveSupport::Deprecation with self, allow other objects to e…

2c690a0

…xtend/include it also. test local deprecation deprecator object Test ActiveSupport::Deprecation when included

carlosantoniodasilva added a commit that referenced this pull request Sep 13, 2012

Merge pull request #6348 from LTe/no_global_depreactations

01ef633

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

LTe commented May 16, 2012

rafaelfranca commented May 16, 2012

jeremy commented May 16, 2012

LTe commented May 18, 2012

jeremy May 18, 2012

jeremy commented May 18, 2012

carlosantoniodasilva May 19, 2012

LTe commented May 19, 2012

paneq commented May 20, 2012

paneq commented May 20, 2012

LTe commented May 21, 2012

paneq commented May 21, 2012

LTe commented May 21, 2012

LTe commented May 23, 2012

jeremy May 23, 2012

LTe commented May 23, 2012

jeremy May 23, 2012

jeremy commented May 23, 2012

paneq commented May 23, 2012

jeremy commented May 23, 2012

LTe commented Jun 7, 2012

LTe commented Jun 29, 2012

steveklabnik commented Jul 1, 2012

LTe commented Jul 2, 2012

steveklabnik commented Jul 2, 2012

paneq commented Jul 9, 2012

jeremy commented Sep 11, 2012

carlosantoniodasilva Sep 11, 2012

carlosantoniodasilva commented Sep 11, 2012

LTe commented Sep 12, 2012

carlosantoniodasilva commented Sep 12, 2012

LTe commented Sep 13, 2012

carlosantoniodasilva commented Sep 13, 2012


		# Default warning behaviors per Rails.env.
		DEFAULT_BEHAVIORS = {

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

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

Conversation

LTe commented May 16, 2012

rafaelfranca commented May 16, 2012

jeremy commented May 16, 2012

LTe commented May 18, 2012

jeremy May 18, 2012

Choose a reason for hiding this comment

jeremy commented May 18, 2012

carlosantoniodasilva May 19, 2012

Choose a reason for hiding this comment

LTe commented May 19, 2012

paneq commented May 20, 2012

paneq commented May 20, 2012

LTe commented May 21, 2012

paneq commented May 21, 2012

LTe commented May 21, 2012

LTe commented May 23, 2012

jeremy May 23, 2012

Choose a reason for hiding this comment

LTe commented May 23, 2012

jeremy May 23, 2012

Choose a reason for hiding this comment

jeremy commented May 23, 2012

paneq commented May 23, 2012

jeremy commented May 23, 2012

LTe commented Jun 7, 2012

LTe commented Jun 29, 2012

steveklabnik commented Jul 1, 2012

LTe commented Jul 2, 2012

steveklabnik commented Jul 2, 2012

paneq commented Jul 9, 2012

jeremy commented Sep 11, 2012

carlosantoniodasilva Sep 11, 2012

Choose a reason for hiding this comment

carlosantoniodasilva commented Sep 11, 2012

LTe commented Sep 12, 2012

carlosantoniodasilva commented Sep 12, 2012

LTe commented Sep 13, 2012

carlosantoniodasilva commented Sep 13, 2012