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 |
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. |
7d5f756 to
ec2ddc4
Compare
Right. I do think it should be enabled by default. |
ec2ddc4 to
c561490
Compare
|
Code and tests all look fine, though I do wonder about the name. Specifically, "undef_import" makes me think of the perl |
|
It is also possible to have an undefined sub that exists (a stub). |
|
I've updated the category to |
pod/perldiag.pod
Outdated
| This is very often the sign of a misspelled package name in a use or | ||
| require statement that has silently succeeded due to a case insensitive |
There was a problem hiding this comment.
This seems to have lost the C<> formatting on the use and require keywords.
Also, require doesn't call import, so should not be mentioned here, but instead no should, because that calls unimport.
Partial revert of f430ad8 (PR #23992) This changes calling an missing 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.
1cc19d6 to
969d598
Compare
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