error messages - do not repeat class names twice in our error messages #20165
Conversation
demerphq force-pushed the yves/no_repeat_package branch 2 times, most recently from 455b620 to e693667 Compare | I really think this should be merged. The cognitive load from the double classname when the classname is long is quite high and I dont think repeating it twice really adds much value. |
| What are peoples' thoughts on this? If we want it and demerphq is busy, I'm happy to rebase it. |
| When considering this, please consider cases where the class name is very long, such as in generated class names or whatnot. Some of what we do seems reasonable when the class name is short, but ends up being counterproductive when it is long. Yves …On Thu, 5 Dec 2024 at 14:38, Richard Leach ***@***.***> wrote: What are peoples' thoughts on this? If we want it and demerphq is busy, I'm happy to rebase it. — Reply to this email directly, view it on GitHub <#20165 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZ5R22XYEPIDYWU67JGXD2EBJORAVCNFSM6AAAAABTCPT5SWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRQGM2TEMBWGA> . You are receiving this because you authored the thread.Message ID: ***@***.***> -- perl -Mre=debug -e "/just|another|perl|hacker/" |
| I generally agree with the principle that each message only needs to name the class once. This PR is a small simple change that implements the idea well. I say go for it. |
Showing the classname twice in the error message seems friendly when the class name is short, but when it is long enough that the line wraps the duplication just increases cognitive load understand the error message. This is especially the case when the value is a total error and contains gibberish or a long binary string or such things, something that is all to common with this type of error. Even with the recently merged eliding of names the duplication means that an error message can be 2k long in the worst case, mostly because of the unnecessary duplication. As a compromise we can simply replace the second invocation of the class name by saying "it" instead, and that is what this patch does. The error message is still friendly, but not repetitive. We could also use "the package" if people preferred. Thus: $ perl -le'("x" x 50)->new()' Can't locate object method "new" via package "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" (perhaps you forgot to load "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"?) at -e line 1. Turns into: $ ./perl -le'("x" x 50)->new()' Can't locate object method "new" via package "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" (perhaps you forgot to load it?) at -e line 1. 
khwilliamson force-pushed the yves/no_repeat_package branch from e693667 to 3cf01b3 Compare 
| Can we also substr the class name in cases where it contains control characters or is longer than a "reasonable" class name ("Reasonable" could come from the longest path the OS allows, for example) It would be nice in cases where you call a method on a string that's not a class name, especially in webby cases, like jpgs or for large json blobs: my $jph_binary = slurp glob "~/pics/kitten.jpg"; $jpg_binary->anything(); |
| @guest20 That's not a bad idea, but I wouldn't do it in this PR. Can you open a new bug? |
That's literally what QUOTEDPREFIX is for. |
Indeed. |
Showing the classname twice in the error message just increases
cognitive load understanding the message when the class/package name
is more than a few components long, and can easily make what should be
a simple one line error message wrap and be unreadable.
We can simply replace the second invocation of the class name by saying
"it" instead, and that is what this patch does. It is still friendly,
but not repetitive.
Thus:
$ perl -le'("x" x 50)->new()'
Can't locate object method "new" via package "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
(perhaps you forgot to load "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"?) at -e line 1.
Turns into:
$ ./perl -le'("x" x 50)->new()'
Can't locate object method "new" via package "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
(perhaps you forgot to load it?) at -e line 1.