class library: improve suggestions in method not found error #4929

telephon · 2020-05-15T12:14:37Z

Purpose and Motivation

The current error fixing suggestion can be misleading. Typically, the receiver is wrong, not the message. So when you call:

nil.flop

you get a suggestion that you mistyped flop, but it happens to be nil which doesn't understand flop:

ERROR: Message 'flop' not understood. Did you mean 'stop'?
RECEIVER:
   nil
ARGS:

This branch improves this situation.

If the receiver is nil, no suggestion is made - this seems like a reasonable thing not to do.

Otherwise, we post this:

"Did you mean '%'? Or should '%' have been called on another receiver?"

So that:

Set[1, 2, 3].flop 

// returns 

ERROR: Message 'flop' not understood by 'Set[ 1, 3, 2 ]'
Did you mean 'stop'? Or should 'flop' have been called on another receiver?
RECEIVER:
Instance of Set {    (0x11bbaff68, gc=78, fmt=00, flg=00, set=02)
  instance variables [2]
    array : instance of Array (0x11b705628, size=6, set=3)
    size : Integer 3
}
ARGS:
CALL STACK:
	DoesNotUnderstandError:reportError
		arg this = <instance of DoesNotUnderstandError>
	Nil:handleError
		arg 
[etc.]

This still isn't optimal in all cases, but catches most "misunderstandings".

Types of changes

general improvement

To-do list

Code is tested
All tests are passing
This PR is ready for review

mossheim · 2020-05-15T12:41:43Z

thanks! but, i don't understand how turning off suggestions for nil solves the problem you raised.

telephon · 2020-05-15T16:38:35Z

I was just updating the text above, is it understandable now?

mossheim

makes sense now that i've had some time to think about it. i think your point is that messages not being understood by nil are far far more often due to a nil being passed where another object is intended, right?

mossheim · 2020-05-16T20:18:52Z

SCClassLibrary/Common/Core/Error.sc

 	}
 	reportError {
 		this.errorString.postln;
+		suggestion.postln;


i really think this should be part of errorString, so it's printed both at the top and bottom of the log

OK, for convenience this makes sense.

mossheim · 2020-05-16T20:20:41Z

SCClassLibrary/Common/Core/Error.sc

 		}
 	}
 	errorString {
-		^"ERROR: Message '" ++ selector ++ "' not understood." ++ suggestion
+		^"ERROR: Message '%' not understood by '%'".format(selector, receiver)


i think this was better as it was. the receiver is already printed out in a more detailed form below this, and this could also lead to massive printouts if the receiver is a large string or collection.

mossheim · 2020-05-16T20:23:00Z

SCClassLibrary/Common/Core/Error.sc

+			// to avoid unhelpful suggestions.
+			if(editDistances[minIndex] <= 3 and: { methodNames[minIndex].similarity(lowerCaseSelector) > 0 }) {
+				suggestedCorrection = methods[minIndex];
+				suggestion = "Did you mean '%'? Or should '%' have been called on another receiver?"


this feels a bit wordy to me. how's this: "Perhaps you misspelled '%', or meant to call '%' on another receiver?"

much better.

telephon · 2020-05-17T11:52:34Z

makes sense now that i've had some time to think about it. i think your point is that messages not being understood by nil are far far more often due to a nil being passed where another object is intended, right?

Yes, it is the most common case, not only for nil, but also for many other cases like the one demonstrated above. I also find that an error system should rather give no suggestions than misleading ones.

mossheim · 2020-05-17T12:05:15Z

I also find that an error system should rather give no suggestions than misleading ones.

for the tools i use that implement this same kind of message, i find it immensely useful, and apparently the maintainers did too. i think we have to disagree here. :)

telephon · 2020-05-17T12:08:59Z

Don't get me wrong, I find these suggestions very useful. They only should be correct in a large majority of cases (and I don't mean that it should not guess!).

telephon · 2020-05-17T12:09:58Z

Better now? I've added a bit of space, this looks less cluttered to me now.

mossheim · 2020-05-17T12:19:54Z

SCClassLibrary/Common/Core/Error.sc

 		receiver.dump;
 		"ARGS:\n".post;
 		args.dumpAll;
 		this.errorPathString.post;
 		if(protectedBacktrace.notNil, { this.postProtectedBacktrace });
 		this.dumpBackTrace;
 		// this.adviceLink.postln;
-		"^^ The preceding error dump is for %\nRECEIVER: %\n\n\n".postf(this.errorString, receiver);
+		"\n^^ %\nRECEIVER: %\n\n\n".postf(this.errorString, receiver);


please keep this as it was, the sections of the printout are very clearly labelled and extra vertical whitespace just leads to more scrolling.

These are only 3 lines out of 40. Maybe give it a second look tomorrow, or we ask others what they think? These things are sometimes a matter of taste, in the end I'm fine with either of course. To me, it is an improvement.

Also I'd really like to get rid of the long "The preceding error dump is for" – it makes it harder to read, and the "^^" are clear indications upward.

i don't want to solve formatting issues by adding extra blank lines in the middle of a printout; it creates a bad precedent, which i explained before here. i would be much more interested in removing unnecessary information, or changing the way the information is presented, but that would happen outside this PR.

OK, I'm not sure I understand your problem, but fine with me! Let's keep this in mind though.

mossheim

thanks!

class library: improve suggestions in method not found error

abc9c7f

mossheim suggested changes May 16, 2020

View reviewed changes

mossheim added the comp: class library SC class library label May 16, 2020

class library: improve error printing, formatting

bb94355

mossheim reviewed May 17, 2020

View reviewed changes

class library: remove blank lines in favor of a future better solution

16f0cab

mossheim approved these changes May 17, 2020

View reviewed changes

telephon merged commit 13fb9a4 into supercollider:develop May 17, 2020

mossheim mentioned this pull request Nov 17, 2020

3.12.0 Discussions #5247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

class library: improve suggestions in method not found error #4929

class library: improve suggestions in method not found error #4929

telephon commented May 15, 2020 •

edited

Loading

mossheim commented May 15, 2020

telephon commented May 15, 2020

mossheim left a comment

mossheim May 16, 2020

telephon May 17, 2020

mossheim May 16, 2020

telephon May 17, 2020

mossheim May 16, 2020

telephon May 17, 2020

telephon commented May 17, 2020

mossheim commented May 17, 2020

telephon commented May 17, 2020 •

edited

Loading

telephon commented May 17, 2020

mossheim May 17, 2020

telephon May 17, 2020

mossheim May 17, 2020

telephon May 17, 2020

mossheim left a comment

class library: improve suggestions in method not found error #4929

class library: improve suggestions in method not found error #4929

Conversation

telephon commented May 15, 2020 • edited Loading

Purpose and Motivation

Types of changes

To-do list

mossheim commented May 15, 2020

telephon commented May 15, 2020

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telephon commented May 17, 2020

mossheim commented May 17, 2020

telephon commented May 17, 2020 • edited Loading

telephon commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

telephon commented May 15, 2020 •

edited

Loading

telephon commented May 17, 2020 •

edited

Loading