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

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

Merged
merged 6 commits into from Mar 15, 2018

Conversation

Projects
None yet
6 participants
@jamshark70
Copy link
Contributor

jamshark70 commented Feb 16, 2018

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.

jamshark70 added some commits Feb 16, 2018

Lang: Restructure Event_IsRest primitive to reduce 'else' nesting
Early tests' true branches use 'return' so 'else' is not needed.
Simplifies future enhancements.

@jamshark70 jamshark70 added this to the 3.9.2 milestone Feb 16, 2018

@jamshark70 jamshark70 changed the base branch from develop to 3.9 Feb 16, 2018

@telephon
Copy link
Member

telephon left a comment

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

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 16, 2018

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

This comment has been minimized.

Copy link
Member

telephon commented Feb 16, 2018

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

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 16, 2018

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.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 16, 2018

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

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 16, 2018

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.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Feb 16, 2018

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.

Lang: Deprecation warning for use of isRest key for event rests
'isRestCount' avoids flooding the post window. First use will always
get the warning, then every 100 thereafter.
@jamshark70

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 17, 2018

@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

This comment has been minimized.

Copy link
Contributor

scztt commented Feb 17, 2018

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

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 17, 2018

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

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Feb 17, 2018

... 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.

@brianlheim

This comment has been minimized.

Copy link
Member

brianlheim commented Mar 1, 2018

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

if(++isRestCount == 1)
post("\nWARNING: Setting isRest to true in an event is deprecated. See the Rest helpfile for supported ways to specify rests.\n\n");
if(isRestCount == 100)
isRestCount = 0;

This comment has been minimized.

@brianlheim

brianlheim Mar 4, 2018

Member

simpler: isRestCount %= 100


slot = array->slots + 1; // scan only the odd items

for (i = 1; i < size; i += 2, slot += 2) {

This comment has been minimized.

@brianlheim

brianlheim Mar 4, 2018

Member

you can eliminate size (just use array->size) and move the declaration of i to the for statement. you could even eliminate i altogether:

for (PyrSlot *slot = array->slots + 1; slot < array->slots + array->size; slot += 2)
SetBool(g->sp, 1);
return errNone;
};
// failing those, scan slot values for a Rest instance or \ or \r

This comment has been minimized.

@brianlheim

brianlheim Mar 4, 2018

Member

this comment is inaccurate (it's Rest, Meta_Rest, \, \r, \rest) - probably more helpful to just focus on intent and say "scan slots for a rest-like value"

}
} // why no 'else'?
// slotSymbolVal nonzero return = not a symbol;
// non-symbols don't indicate rests, so, ignore them.

This comment has been minimized.

@brianlheim

brianlheim Mar 4, 2018

Member

I would like to see this (lines 600-615) moved out into a separate function if you have the time. That way you can localize slotSym and only have to write one return statement. bool slotIsRestlike(PyrSlot* slot) or similar. This would make future changes easier to understand (clarifies control flow, abstracts and names subtasks, reduces number of local variables)


if (isKindOfSlot(arraySlot, class_array)) {
PyrSlot key, typeSlot;
PyrSymbol *typeSym;
// test 'this[\type] == \rest' first
// easy tests first: 'this[\type] == \rest'

This comment has been minimized.

@brianlheim

brianlheim Mar 4, 2018

Member

these comments make it much easier to understand what's going on, thanks!

@patrickdupuis

This comment has been minimized.

Copy link
Contributor

patrickdupuis commented Mar 13, 2018

@jamshark70 do you think you will have time to finish this PR in the coming days/weeks? It would be great if it was included in 3.9.2.

@jamshark70

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Mar 13, 2018

do you think you will have time to finish this PR in the coming days/weeks?

Maybe...? Hard to say. The spring semester starts late in China, and it's always chock full of crunchy administrative goodness. I'm a bit snowed under for the next few days.

@brianlheim
Copy link
Member

brianlheim left a comment

Pushed the changes I requested (@jamshark70 privately said this was OK), the tests all pass. Thanks again for this!

@jamshark70

This comment has been minimized.

Copy link
Contributor

jamshark70 commented Mar 14, 2018

Pushed the changes I requested (@jamshark70 privately said this was OK)...

I did indeed. Thanks for that. It's an inconvenient time -- I appreciate the extra hands.

@brianlheim brianlheim merged commit 6a97779 into supercollider:3.9 Mar 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment