Lang: Add event[\isRest] == true test to Event_IsRest primitive #3521

jamshark70 · 2018-02-16T01:17:04Z

http://new-supercollider-mailing-lists-forums-use-these.2681727.n2.nabble.com/3-9-1-and-isRest-tp7638027.html

A user made the unfortunate inference that \isRest, true in a Pbind was a supported way to mark an event as a rest, not realizing that it was a private implementation detail that was subject to change. Unfortunately, his private code base now contains some 20,000 occurrences of \isRest.

I could make a strong case that this is de facto a legacy way to write rests, and we shouldn't burn users who got the wrong idea. (It is, in fact, not the users' fault that sclang does nothing to prevent public use of private entities.) Hence, this PR.

One could also make a case that \ and \r were previously legitimately supported, but \isRest, true was not.

I think on balance, the maintenance burden for adding legacy \isRest support is extremely low and the benefit to users much greater. So I'm suggesting this for 3.9.2 or .3. (I'm aware that we shouldn't take this as precedent to keep adding to the primitive in the future because of misunderstandings. I'd like this to be a one-off.)

Two commits. The first removes a layer of else structure -- if(cond) { return ... } else { ... } does not actually need the else. The second uses the simplified structure to add another test. I've also updated the event unit tests.

All TestAbstractFunction unit tests pass.

Early tests' true branches use 'return' so 'else' is not needed. Simplifies future enhancements.

telephon

The only thing I am worried is that we have now too many ways to write the same thing.

This again adds another way of thinking about rests. Would I expect the following:

a = (isRest:false, dur:Rest(1));
a.isRest // true
a[\isRest] // false

Don't take this as a strong opposition. I just want to avoid adding up complications in the future.

jamshark70 · 2018-02-16T08:01:21Z

The only thing I am worried is that we have now too many ways to write the same thing.

Yes and no... \isRest, true will not be, and wasn't, officially supported for user code.

Documentation needs to be more strict about "These are the supported ways to write Rests; anything else is at your own risk and there are no promises of it working in the future." IMO \isRest should not make the supported list.

telephon · 2018-02-16T09:14:51Z

Ah I see, so you are not proposing to promote it, but to add it as legacy support. So we can only hope it doesn't spread :)

jamshark70 · 2018-02-16T15:02:07Z

Ah I see, so you are not proposing to promote it, but to add it as legacy support.

Correct.

With regard to your example, I'm pretty sure we already document that the correct way to ask if an Event is a rest is using the isRest method. The fact that .at(\isRest) returns the opposite result in that case is irrelevant because the documentation discourages this usage (and users shouldn't expect the \isRest key to have any particular meaning anyway).

We can try up to a point to protect users from themselves, but if the documentation says "do it this way" and a user does something else, said user doesn't have much standing to complain about the results.

mossheim · 2018-02-16T15:16:03Z

Would you consider adding a deprecation warning? IMO documenting that this usage is deprecated may not be loud enough for some. I remember that in the case of OSCresponder there were a number of users for whom a deprecation warning was a separate concept from documented deprecation. I would like to avoid a similar conversation in the future if possible.

jamshark70 · 2018-02-16T20:37:03Z

Hm... I guess a deprecation warning would have to go into the Event_IsRest primitive? The only legitimate way to deprecate a particular usage of an Event key is to catch it where the deprecated usage is implemented.

If you're not following the mailing list thread, it's worth a read. The original poster has some valid considerations. I'm still pondering, but I have some ideas how to proceed.

mossheim · 2018-02-16T21:07:50Z

Hm... I guess a deprecation warning would have to go into the Event_IsRest primitive?

Yeah, it's messy, but I'm not sure the current setup allows for much else.

'isRestCount' avoids flooding the post window. First use will always get the warning, then every 100 thereafter.

jamshark70 · 2018-02-17T15:37:13Z

@brianlheim OK, here's an attempt. I worried about flooding the post window, so I added logic to post the warning every hundred uses. Maybe that's not necessary, but if there's high event density, I think it's rude to post potentially dozens/hundreds of identical messages a second, e.g.

(
p = Pbind(
	\isRest, 0.9.asPattern.coin,
	\freq, Pexprand(300, 600, inf),
	\dur, 0.002
).play;
)

p.stop;

scztt · 2018-02-17T21:39:17Z

Rather than burdening with deprecation warnings, I wonder if there's a way we can constrain the fix to something at the classlib level. If this were possible, then the "legacy" behavior could be implemented as a quark, and users depending on the old behavior could simply monkeypatch it back in. If it requires backend code, we might even have two primitives (a current one and a legacy one), and the quark would simply redirect to the legacy primitive.

BTW, I think the proposed solution is fine - but more generally I think the "legacy behavior quark" model might be a good one for dealing with this kind of situation.

jamshark70 · 2018-02-17T22:54:44Z

No, wait... there is one option to do it in the classlib. It's a bit ugly but it would work.

Briefly, the quark would have a prIsRest method to call the primitive, and use isRest to check the key. User's responsibility, at your own risk, no support if it breaks in the future.

As for my comment about extensibility, I'll back off a bit... after all, it conflicts with my comments elsewhere about the risks of adding rest logic as a matter of precedent. (So my own views evolved.) I'll remove my previous comment, but that doesn't rescind the email, so I have to mention it.

jamshark70 · 2018-02-17T23:15:07Z

... but, the problem with the legacy quark idea is, if it becomes popular, it won't be long before someone wants two of them at the same time. Quarks offer no way to merge conflicting versions of the same method (nor should they).

Sorry if it looks like I'm indecisive... I'm not really sure what to do here. I'm not satisfied to tell the user who raised the question "sorry, you're out of luck," but I'm more aware than I used to be of the risks of opening up the Rest logic.

I don't really like deprecation warnings either, but I'm kind of still leaning toward that. Users may need a gentle push to update their code. Then we could even remove it in 3.10 or 3.11, after enough time for users to adjust.

mossheim · 2018-03-01T03:10:00Z

I can review this now if we're ok with this approach. I think the approach here looks fine.

mossheim · 2018-03-04T17:27:08Z