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

Include gem name and deprecation horizon when calling the deprecation behaviors #28800

Merged
merged 1 commit into from Apr 19, 2017

Conversation

Projects
None yet
5 participants
@wvanbergen
Contributor

wvanbergen commented Apr 19, 2017

Summary

We are planning to use ActiveSupport::Deprecation in our own code base to mark deprecated behavior. We have some tooling subscribing to the ActiveSupport::Notifications that are generated by the deprecations.

However, for our tooling we would like to be able to make sure we can differentiate between the source of the deprecation. ActiveSupport::Deprecation already has useful context for this: gem name, and deprecation horizon. Unfortunately, these pieces of metadata are not made available to the behavior lambdas.

This PR exposes them as extra arguments, and makes sure those arguments are passed on to the ActiveSupport::Notification we create. As part of this, we also dynamically generate the notification name based on the gem name. It was hardcoded to deprecations.rails before. All the other predefined behaviors are unaffected by this.

cc @rafaelfranca @casperisfine

@kaspth

Since this changes the number of arguments that can be passed to a custom installed behavior, we'd need to inspect the arity and coerce those that don't support the more elaborate version. Perhaps something like this:

def behavior=(behavior)
  @behavior = Array(behavior).map { |b| DEFAULT_BEHAVIORS[b] || arity_coerce(b) }
end

private
  def arity_coerce(behavior)
    if behavior.arity >= 2
      behavior
    else
      -> message, callstack, _, _ { behavior.call(message, callstack) }
    end
  end
@@ -119,7 +119,7 @@ def test_raise_behaviour
callstack = caller_locations
e = assert_raise ActiveSupport::DeprecationException do
ActiveSupport::Deprecation.behavior.first.call(message, callstack)
ActiveSupport::Deprecation.behavior.first.call(message, callstack, "horizon", "gem")

This comment has been minimized.

@kaspth

kaspth Apr 19, 2017

Member

Alludes to backwards incompatibility: custom installed behaviors can raise ArgumentError's now.

behavior = ActiveSupport::Deprecation.behavior.first
begin
events = []

This comment has been minimized.

@kaspth

kaspth Apr 19, 2017

Member

Why define this as an array when we expect there to only be one? If it wasn't the right message, we'd eventually find out in the latter tests.

E.g. we could opt for _, event = ActiveSupport::Notifications.subscribe("deprecation.my_gem.custom", &:itself).

This comment has been minimized.

@wvanbergen

wvanbergen Apr 19, 2017

Contributor

This doesn't work because the itself method doesn't have the correct arity.

I also kind of like the explicitness of the array approach. Less obscure on how it works, and makes sure we're not actually overriding another earlier deprecation notification without noticing.

activesupport/lib/active_support/deprecation/reporting.rb Outdated
@@ -5,8 +5,6 @@ class Deprecation
module Reporting
# Whether to print a message (silent mode)
attr_accessor :silenced
# Name of gem where method is deprecated
attr_accessor :gem_name

This comment has been minimized.

@kaspth

kaspth Apr 19, 2017

Member

This module looks like it's public API so we can't just yank this.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 19, 2017

Member

Hardly it will break any behavior unless someone is including Reporting in their own class, what I'd not recommend anyway since this method depends on the others in Deprecation. I think this change is fine since the public API (ActiveSupport::Deprecation) did't changed.

This comment has been minimized.

@wvanbergen

wvanbergen Apr 19, 2017

Contributor

I'll just revert this change. I noticed two other accessors defined on other modules as well. I feel they should be part of the main module given that it directly uses them in the initializer, but it's kind of out of scope for this PR.

@maclover7

Agree with what @kaspth said -- we have to be careful with backwards compatibility here. Can you add a CHANGELOG.md entry, since we are modifying public API?

activesupport/lib/active_support/deprecation.rb Outdated
@@ -29,6 +29,9 @@ class Deprecation
# The version number in which the deprecated behavior will be removed, by default.
attr_accessor :deprecation_horizon
# Name of gem where method is deprecated

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 19, 2017

Member

Missing . in the end of the text (in the method below too)

activesupport/lib/active_support/deprecation/behaviors.rb Outdated
ActiveSupport::Notifications.instrument("deprecation.rails",
message: message, callstack: callstack)
notify: ->(message, callstack, deprecation_horizon, gem_name) {
notification_name = "deprecation.#{gem_name.underscore.tr('/', '.')}"

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 19, 2017

Member

Maybe _ instead of .? We don't want to mix the gem namespace with the notification namespace. deprecation.action_controller.test_helper seems more like a test_helper notification namespace instead of the gem namespace.

activesupport/lib/active_support/deprecation/reporting.rb Outdated
@@ -5,8 +5,6 @@ class Deprecation
module Reporting
# Whether to print a message (silent mode)
attr_accessor :silenced
# Name of gem where method is deprecated
attr_accessor :gem_name

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 19, 2017

Member

Hardly it will break any behavior unless someone is including Reporting in their own class, what I'd not recommend anyway since this method depends on the others in Deprecation. I think this change is fine since the public API (ActiveSupport::Deprecation) did't changed.

@wvanbergen

This comment has been minimized.

Contributor

wvanbergen commented Apr 19, 2017

Thanks for the fast reviews! Pushed some updates.

Send deprecation horizon and gem name as arguments to deprecation hea…
…vier handler, and make sure they are used for the ActiveSupport::Notifications message.

@rafaelfranca rafaelfranca merged commit 2b18153 into rails:master Apr 19, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 19, 2017

❤️ 💚 💙 💛 💜

@maclover7 maclover7 removed the needs work label Apr 19, 2017

@kirs kirs deleted the Shopify:deprecation_includes_gem_name_and_version branch Apr 20, 2017

jfragoulis pushed a commit to jfragoulis/rails that referenced this pull request Feb 23, 2018

John Fragoulis
Correct ActiveSupport::Deprecation::Behavior#behavior= documentation
The callback parameters need to reflect changes after
rails#28800
ActiveSupport::Notifications.instrument("deprecation.rails",
message: message, callstack: callstack)
notify: ->(message, callstack, deprecation_horizon, gem_name) {
notification_name = "deprecation.#{gem_name.underscore.tr('/', '_')}"

This comment has been minimized.

@Edouard-chin

Edouard-chin Mar 22, 2018

Contributor

Sorry for digging this up after one year, adding the gem_name and the deprecation_horizon to the event is a great add however generating dynamically the notification name has one major disadvantage because that means we need to know in advance which gems have deprecations in order to keep track of them.

class MySubscriber < ActiveSupport::Subscriber
  attach_to :rails
  # Prior to this PR, any gems using AS deprecations could be tracked thanks to this subscriber.
  # Now we can't because we'd need to attach the subscriber to other namespace which we don't know, thus the concept of "notifying" when a deprecation occur (no matter inside which gem) no longer exists

  def deprecation(event)
    ...
  end
end

My suggestion would be to use a static namespace like before (maybe change the rails namespace because it's not super accurate since any gems can trigger deprecation)

This comment has been minimized.

@wvanbergen

wvanbergen Mar 22, 2018

Contributor

I was not a big fan of this, but this was required for backwards compatibility. I would have preferred to simply include the gem name as a parameter rather than in the event name.

I think you can use regular expressions for subscriptions, at least with the lower level API ActiveSupport::Notifications::Fanout API. That may not be propagated to ActiveSupport::Subscriber though.

This comment has been minimized.

@Edouard-chin

Edouard-chin Mar 22, 2018

Contributor

Ah ok I see thanks for the explanation, and TIL indeed the lower level API Fanout can be passed a regex. I'm going to open a PR to make it possible for AS::Subscriber as well

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 22, 2018

Member

we need to know in advance which gems have deprecations in order to keep track of them.

That was exactly the reason why we did this. We should not thread deprecation in Rails in the same way we treat deprecation in other gems. At the time of a Rails upgrade, we don't care if the gem X also has deprecations, only if Rails has deprecation. Because of this we need to have different subscriber to each gem so we can chose the behavior we want based on the gem.

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