Preserve and utilize Warning categories

[thoughtbot-summer]

Currently Deprecation Toolkit does not utilize Warning's :deprecated category, and in fact it nullifies any category that is set when calling super. This change will preserve the category that is passed to Warning.warn; it will automatically enable the :deprecated category, which the user can disable or restore with a new config option; and it will treat any warning with the :deprecated category as a deprecation, if the category is enabled.

[thoughtbot-summer]

I have signed the CLA!

[etiennebarrie]

Thanks for your contribution!

We should definitely preserve and pass the :category keyword argument, as well as using it to ignore warnings that are deprecated if the category is disabled.

But we don't need DeprecationToolkit::Configuration#warning_deprecated_category to set Warning[:category], it can be done by the application directly on Warning no? Or am I missing something?

[thoughtbot-summer]

But we don't need DeprecationToolkit::Configuration#warning_deprecated_category to set Warning[:category], it can be done by the application directly on Warning no? Or am I missing something?

It's definitely possible! It would be a pretty straightforward change, and it would feel much more elegant to me. But it would also be a breaking change. At the moment, since Deprecation Toolkit nullifies a warning's category, it effectively enables the :deprecated category unconditionally. In order to allow users to enable/disable the category themselves (whether via Warning[:deprecated] = … or via the -W:deprecated/-W:no-deprecated Ruby flags), it would be necessary for Deprecation Toolkit not to attempt to enable that category at all, which means that anyone who upgrades to the new version of this gem would need to enable the category.

[etiennebarrie]

So the thing I'm seeing is that removing the explicit setting of Warning[:deprecated] is not causing any compatibility issue out of the box with Rails/minitest. The reason is that rails/test_help, at the top of the default generated test/test_helper, requires active_support/testing/autorun, which calls Minitest.autorun which enables the :deprecated category.

Testing with RSpec, I also don't see any regression regarding :deprecated warnings when we remove the explicit setting of Warning[:deprecated]. The warning comes in with category :deprecated, Warning[:deprecated] is false, so your new condition is false, and it falls back to the previous condition, so if you had an empty regex to make sure all warnings would be caught, it would still be caught.

Also worth noting that Ruby still sends the warning to Kernel.warning if the category is not enabled, so I really don't think we need to enable those.

Ruby script that shows warnings are sent even if the category is not enabled

module WarningExt
  def warn(message, category: nil)
    super "#{"[#{category.upcase}] " if category}#{message}"
  end
end

Warning.extend(WarningExt)

p deprecated: Warning[:deprecated], experimental: Warning[:experimental]
warn "foo"
warn "deprecated", category: :deprecated
warn "experimental", category: :experimental
warn "bar", category: :bar rescue p $!

outputs when called with ruby test.rb:

{:deprecated=>false, :experimental=>true}
foo
[DEPRECATED] deprecated
[EXPERIMENTAL] experimental
#<ArgumentError: unknown category: bar>

If we enable warnings with ruby -w test.rb, we get the same output except that the :deprecated category is enabled.

Taking a step back:

1. We're currently ignoring the category, which is slightly annoying if you want to ignore such warnings (e.g. you want to use Ractors, you disable Warning[:experimental], and you still have to configure Deprecation Toolkit to ignore those).
2. Given the name and purpose of this gem, it could make sense to handle all :deprecated warnings as deprecations.

1 feels like we should respect the Warning category, and check whether the category is enabled or not before handling warnings as deprecations. This would be a breaking change in some cases because :deprecated warnings are not enabled by default, which means we would ignore some warnings that were previously collected.
2 makes me think we could turn that into a feature, by enabling :deprecated warnings when deprecation toolkit is required.

In general, I wouldn't be in favor of configuring things "behind the back" of the application authors (hence why I asked about this) but given that in test/development, using deprecation toolkit, is truly the best time to enable deprecation warnings and track them in order to take care of them some time before the deprecated behavior is removed, I feel like it's fair to enable them.

So by combining those two, maybe what we want is to check if the warning is of a category that's enabled, and also enable Warning[:deprecated]. I think these two combined prevent any compatibility issue.