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

nil respondsTo asSynthDef, but gives an error when called #1144

Open
capocasa opened this issue Jul 5, 2014 · 11 comments
Open

nil respondsTo asSynthDef, but gives an error when called #1144

capocasa opened this issue Jul 5, 2014 · 11 comments
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" enhancement
Milestone

Comments

@capocasa
Copy link
Member

capocasa commented Jul 5, 2014

(
nil.respondsTo(\asSynthDef); // returns true
nil.asSynthDef; // throws an error
)

Presumably, nil.respondsTo(\asSynthDef) should return false.

@telephon
Copy link
Member

telephon commented Jul 6, 2014

This is because Object implements a special error message.
There is no reasonable way to make nil.respondsTo(\asSynthDef) return false.

    asSynthDef {
        error("Cannot convert this object to a SynthDef: " + this);
        this.dump;
        ^nil
    }

I suppose one could remove the implementation of asSynthDef altogether, if of course we accept the less informative error.

@jamshark70
Copy link
Contributor

Oh dear, this whole thing is quite ugly, isn't it?

Object:asSynthDef doesn't throw an error; it only prints one. This means, if you want to suppress the message using try, you... can't because no error is actually thrown!

I wonder if we shouldn't establish a convention, that a message appearing as "ERROR" in the post window should only come from an actual instance of Error being thrown? That would mean getting rid of the (IMO misleading) String:error method.

Since execution continues after the so-called-but-not-really error, I'd suggest one of two fixes:

  • Make it throw a real Error.
  • Or, have it print a warning -- i.e., use warn instead of error. (As we can see from the initial message reporting the issue, the fact that the text in the post window says ERROR is itself misleading -- it made Carlo think an error was thrown, but that's not the case.)

@capocasa
Copy link
Member Author

capocasa commented Jul 7, 2014

Thanks for your replies, @telephon and @jamshark70!

I would definitely vote in favor of that convention, error handling has been a continuing source of confusion to the point that I avoid it where I can, which lead to less informative error messages in my library.

Using the lib I am working on as a reference, I would favor using a real error, simply because it can be caught; also, mixing nil-checks with try/catch tends to get a bit brittle.

@capocasa
Copy link
Member Author

capocasa commented Jul 7, 2014

PS: Of course I understand only too well how these kinds of inconsistencies can "grow" over time, but I do believe they are a major stumbling block for those trying to learn the language.

@danstowell
Copy link
Member

I would be in favour of the change. However, it's clear that it's relatively likely to break some people's pieces. (They could of course add "try" statements but that's not the point, when you're thinking about long-term preservation of works.) On balance, and especially since the method looks like throwing an error was kinda intended, I'd be in favour of making the change but announcing it loudly.

@capocasa
Copy link
Member Author

capocasa commented Jul 7, 2014

I agree that long-term preservation of works is quite important.

Perhaps it would make sense find all pseudo-errors (starting with 'grep "ERROR" SCClassLibrary/* -r' or something) and replace them with something like this?

old code:

"ERROR: This is wrong".postln;

new code

Error.softReport(Error("This is wrong"));

// in class Error
softReport {
    arg error;
    if(Platform.legacyErrors) {
        ("ERROR:"+error.what).postln;
        "Warning: This will throw an error in a future SC version.".postln;
    }{
        error.throw;
    }
}

This would bloat things a little for a while, but the whole thing could be removed when the future streamlined version lands. Resources permitting, the legacy version might even be kept around for those who do not wish to update their works.

@timblechmann
Copy link
Contributor

I would be in favour of the change. However, it's clear that it's
relatively likely to break some people's pieces. (They could of course
add "try" statements but that's not the point, when you're thinking
about long-term preservation of works.) On balance, and especially since
the method looks like throwing an error was kinda intended, I'd be in
favour of making the change but announcing it loudly.

move Nil-asSynthDef to a backwards-compatibility quark?

@crucialfelix
Copy link
Member

I can't think of any case where somebody would depend on this behavior.
but I would give it a long thought.

moving it to a backward compatibility quark just fragments the code base
worse than it already is.

anything that currently posts ERROR that doesn't raise should be changed to
"message".warn;

On Mon, Jul 7, 2014 at 8:13 PM, Tim Blechmann notifications@github.com
wrote:

I would be in favour of the change. However, it's clear that it's
relatively likely to break some people's pieces. (They could of course
add "try" statements but that's not the point, when you're thinking
about long-term preservation of works.) On balance, and especially since
the method looks like throwing an error was kinda intended, I'd be in
favour of making the change but announcing it loudly.

move Nil-asSynthDef to a backwards-compatibility quark?


Reply to this email directly or view it on GitHub
#1144 (comment)
.

..
http://soundcloud.com/crucialfelix
http://github.com/crucialfelix
.

@telephon
Copy link
Member

@carlocapocasa and all

I think there isn't too much harm leaving it in, maybe changing it to .warn.

Instead of checking for if(x.respondsTo(\asSynthDef)) { … } which isn't that good style usually anyway, you can just write:

synthDef = x.asSynthDef; 
if(synthDef.isNil) { 
Error("Failed to create a SynthDef from %".format(x)).throw 
};

@jamshark70
Copy link
Contributor

Hi Julian --

One of the points raised here is that Object:asSynthDef posts the warning whether you want it to, or not. Unlike a true Error that you can catch, the only way to prevent the warning is to avoid calling asSynthDef on any object that will fall back to Object's implementation:

if(thing.findRespondingMethodFor(\asSynthDef).ownerClass != Object)

You may be right that it may not be worth breaking backward compatibility over this, but the issue does highlight that we don't really have a consistent way of handling non-fatal error cases: sometimes we throw an error, sometimes we post a warning, and sometimes we just return nil and let the caller decide what to do. It's an area that could use a cleanup in SC4.

@telephon
Copy link
Member

Hi James,

yes, a cleanup, perhaps with verbosity levels, would make a lot of sense. Also the post window code formatting could benefit from some extra thought. Probably the best way to proceed is to use warn in all such cases and have it bubble up.

@scztt scztt added this to the 3.7 milestone Mar 15, 2015
@scztt scztt added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label Mar 15, 2015
@telephon telephon modified the milestones: 3.8, 3.7 Jun 15, 2015
@nhthn nhthn modified the milestones: 3.8, 3.9 Jan 15, 2017
@nhthn nhthn modified the milestones: 3.9, 3.9.x Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" enhancement
Projects
None yet
Development

No branches or pull requests

8 participants