Skip to content
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

Closed

Conversation

demerphq
Copy link
Collaborator

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.

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.
@demerphq demerphq force-pushed the yves/deprecated_subcategories_silence_autodie_warnings branch from 496b887 to 2d705ed Compare February 25, 2023 16:30
@book
Copy link
Contributor

book commented Feb 25, 2023

As commented in #20337 (comment), the PSC discussed if it was worth having subcategories for deprecated.
(See PSC #084.)

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 no warnings 'deprecated'; they are explicitly voiding the warranty on their code: the next Perl upgrade might make it explode. In that context, picking up which deprecation to silence seems futile.

Therefore, I think the fix should be on autodie and its tests, rather than on warnings.

@demerphq
Copy link
Collaborator Author

demerphq commented Mar 6, 2023

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

@haarg
Copy link
Contributor

haarg commented Mar 6, 2023

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.

@demerphq
Copy link
Collaborator Author

demerphq commented Mar 6, 2023

That is exactly the type of thing that will be encouraged by providing a separate category for a deprecation like this.

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.

@demerphq demerphq closed this Mar 9, 2023
@demerphq demerphq deleted the yves/deprecated_subcategories_silence_autodie_warnings branch March 9, 2023 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants