Open thoughtbot-summer opened 1 year ago
I have signed the CLA!
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?
But we don't need
DeprecationToolkit::Configuration#warning_deprecated_category
to setWarning[:category]
, it can be done by the application directly onWarning
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.
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.
Taking a step back:
Warning[:experimental]
, and you still have to configure Deprecation Toolkit to ignore those).: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.
Currently Deprecation Toolkit does not utilize
Warning
's:deprecated
category, and in fact it nullifies any category that is set when callingsuper
. This change will preserve the category that is passed toWarning.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.