-
Notifications
You must be signed in to change notification settings - Fork 555
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
warnings.pm - support deprecated::smartmatch category #20855
warnings.pm - support deprecated::smartmatch category #20855
Conversation
Currently we seem to lack a way to have a subcategory under deprecated. It seems reasonable to me that people might want to disable a specific subcategory warning while leaving the rest in place. This patch allows that. Note that both no warnings "deprecated"; and no warnings "deprecated::smartmatch"; work to disable the warning. Really this needs tests, but this will shut up autodie warnings, so we can do the tests for this later. Also we should go through and enumerate all the deprecated subcategories and switch to using them. Deprecated warnings shouldn't be "all or nothing". Again, I think that should happen after this is merged.
496b887
to
2d705ed
Compare
As commented in #20337 (comment), the PSC discussed if it was worth having subcategories for Since the goal of deprecation warnings is to encourage people to stop using the deprecated constructs completely, if felt like adding the option to silence some deprecation warnings was counter-productive. Once someone starts using Therefore, I think the fix should be on |
@book - I think that it is taking a long time to get this fixed. We shouldnt have deprecation warnings our build for a long period of time. Could you please work with @toddr to get a solution to this in place soon? Otherwise I think we should merge this for now until you can get it sorted out. FWIW I do not really agree with the argument you put forth about deprecation categories, and I think it should have been discussed on perl5-porters before PSC made a "decision" on it. IMO PSC shouldn't be making decisions about things without giving perl5-porters a chance to voice an opinion. And I definitely would have voiced an opinion on this. The position you have taken is that it is better to allow all deprecations than it is to allow a specific deprecation. I find it hard to rationalize that it is better to take more risk, in an open form, than it is to allow a specific risk without allowing others to creep in. I don't understand how it could be counterproductive either. Seems to me we lack a proper list of deprecations and that the warnings sub categories would be a great place to manage them. It would also provide a great way to clearly feedback to the user "You are using a deprecation category that no longer exists because the feature has been removed". Anyway, regardless, this has been warning for sometime, since the PSC is the reason this isnt being merged could you take it on yourselves to get this resolved sooner than later? This has been warning for over 10 days now. |
Adding this warning and allowing autodie to silence it as the current patch does just means that it will break in the future when smartmatch is removed. That is exactly the type of thing that will be encouraged by providing a separate category for a deprecation like this. I've provided a patch for autodie that does a more comprehensive fix that won't break in the future. Dual-Life/autodie#117 It could use a review. |
I don't see that as a problem. Right now someone would silence ALL the deprecation categories, and then when the feature is actually removed we wouldn't know which one that they had intended to silence or whether they can remove the no warnings statement or not. If the item was deprecated specifically then we would know "oh, you are trying to ignore deprecations from X, but X is no longer supported, so you need to a) remove the no warnings, AND b) remove the use of the feature", and we could provide a nice explanation of what they should look for and do. With only a generic category we don't know if the no warnings statement can be removed or not. For all we know the no warnings statement should stay as there might be a new deprecation category that should still be ignored. IMO, more information about the developers intent will always allow us to do a better job than less. I actually think there is a case to be made that we shouldnt have a generic deprecation category at all, as at least some people might be inclined to say "well ill just silence all deprecations in my code always, and let it break when it breaks". As for the immediate case, I don't really have a strong opinion of how we silence these warnings, I just think it is unreasonable to leave code in blead producing deprecation warnings for so long, especially when we have a way to eliminate the warning for the time being. FWIW, My view is that we shouldn't deprecate something in core at all until core itself builds without any deprecation warnings. Eliminating any use of deprecated features in cpan/ and dist/ should be a precondition for merging in a new deprecation case. If it proves difficult to get the upstream modules that use the deprecated features fixed that is a strong sign that the deprecation is more troublesome than we realize. For instance, Test-Simple is still not adjusted for either the apostrophe change nor the smartmatch change. Id say that both should have been resolved before we deprecated the features. It has been over a month since the deprecation was reported to the Test-More/test-more repo in Test-More/test-more#903, fwiw, i pushed Test-More/test-more#904 to fix this. |
Currently we seem to lack a way to have a subcategory under deprecated. It seems reasonable to me that people might want to disable a specific subcategory warning while leaving the rest in place. This patch allows that. Note that both
and
work to disable the warning. Really this needs tests, but this will shut up autodie warnings, so we can do the tests for this later. Also we should go through and enumerate all the deprecated subcategories and switch to using them. Deprecated warnings shouldn't be "all or nothing". Again, I think that should happen after this is merged.