make calling an undefined import method a warning #24059
Conversation
|
I chose |
pod/perldiag.pod
Outdated
| Another common reason this may happen is when mistakenly attempting to | ||
| import or unimport a symbol from a class definition or package which | ||
| does not use C<Exporter> or otherwise define its own C<import> or | ||
| C<unimport> method. |
There was a problem hiding this comment.
I think it would be beneficial to include examples of at least the import sub-case for each of these two cases. The wording is sufficiently complex that I can't easily articulate the difference between the two.
There was a problem hiding this comment.
What kind of example do you have in mind?
There was a problem hiding this comment.
In the first paragraph of the proposed re-write of pod/perldiag.pod (You called the ...), the calling program -- let's call it Program A -- mistakenly thinks that package B either defines its own import method or has an import method in its inheritance graph.
In the second paragraph of the proposed re-write (Another common reason ...), the calling program mistakenly thinks that package B either defines its own import method gets an import method from Exporter.
Isn't the second case just a subset of the first case? If it is not, then we should have examples which clearly distinguish the two cases.
There was a problem hiding this comment.
The first paragraph starts with a direct description of what happened - no import method was found. Then it mentions a common case that could cause this - an incorrect package due to using the wrong case. The second paragraph describes another case, where the package is correct but has no import method when the user was expecting one.
The cause here is something missing. An example of something missing won't be easy to demonstrate in any way that makes sense without additional text, so I'm not sure the example really helps. If you have an example you think helps with this feel free to propose one.
|
Should this warning be enabled by default? I believe the original intent of the fatalisation was catch mistakes like: on case-insensitive file systems. Making it require |
haarg
force-pushed
the
haarg/undefined-import-warning
branch
4 times, most recently
from
304a245 to
7d5f756
Compare
|
I've switched the warning to be enabled by default since it is likely to impact places where warnings haven't been enabled yet. It has also already been enabled by default as a deprecation for a couple releases. |
Partial revert of f430ad8 (PR #23992) This changes calling an undefined import or unimport method with arguments back to a warning, but not a deprecation. Making this fatal had a very large amount of fallout for extremely marginal gains. It has been decided that a warning is enough, and making it fatal in the future will not be pursued. Fixes #24058
haarg
force-pushed
the
haarg/undefined-import-warning
branch
from
7d5f756 to
ec2ddc4
Compare
Right. I do think it should be enabled by default. |
| ######## | ||
| use warnings 'undef_import'; | ||
| Some::Package->import("bar"); | ||
| EXPECT | ||
| Attempt to call undefined import method with arguments ("bar") via package "Some::Package" (Perhaps you forgot to load the package?) at - line 2. | ||
| ######## | ||
| use warnings 'undef_import'; | ||
| Some::Package->unimport(1.234); | ||
| EXPECT | ||
| Attempt to call undefined unimport method with arguments ("1.234") via package "Some::Package" (Perhaps you forgot to load the package?) at - line 2. |
There was a problem hiding this comment.
Doesn't test that the warning is enabled by default.
Or that disabling it works.
Partial revert of f430ad8 (PR #23992)
This changes calling an undefined import or unimport method with arguments back to a warning, but not a deprecation. Making this fatal had a very large amount of fallout for extremely marginal gains. It has been decided that a warning is enough, and making it fatal in the future will not be pursued.
Fixes #24058