I can't really work with "the comments are confusing" and I don't really see that the characterization of the commit messages is accurate. I can try to reword them in a manner that puts less stress on the state before the change.
https://codereview.appspot.com/12432043/diff/6001/input/regression/articulation-syntax-change.ly File input/regression/articulation-syntax-change.ly (right): https://codereview.appspot.com/12432043/diff/6001/input/regression/articulation-syntax-change.ly#newcode3 input/regression/articulation-syntax-change.ly:3: texidoc = "The syntactic class of articulations can be changed within On 2013/08/05 19:21:45, Keith wrote:
It is confusing to put it this way.
Well, I can try my hand of writing less gobbledygook here.
We could always redefine post events to create an empty chord trill = <>\trill Now we can redefine ) as well
I'd prefer not to drag unrelated commands into the description, even if just as an example.
It is not clear what functionality you are protecting from regression.
Redefining ) in a manner that changes it from post-event to standalone event: previous to that patch, the syntax of ) was fixed.
Probably this added regression test is not needed, because the usual definition
and use
of ")" tests the new code.
When it is used for providing the old semantics, yes. We have more or less a policy that new functionality should at least be visible in the regtests. Should I rather move this to a snippet? https://codereview.appspot.com/12432043/diff/6001/lily/lexer.ll File lily/lexer.ll (right): https://codereview.appspot.com/12432043/diff/6001/lily/lexer.ll#newcode153 lily/lexer.ll:153: SPECIAL \\.|[][|()~] On 2013/08/05 19:21:45, Keith wrote:
'special' is vague. Maybe POST_EVENT_CHAR
The whole point is that those characters behave like a backslashed identifier: they will be postfix if defined as a postfix event, or standalone when defined as standalone event. So POST_EVENT_CHAR would be quite the opposite from what this is about. For example, this makes | and ~ very much equivalent from the view of lexer and parser, but due to the assigned default definitions, ~ is post-event and | is not. I can dig into TeX terminology and call this "active", but I thought "special" fit rather well. Emacs syntax tables call this "symbol". In contrast to TeX's active characters, they also combine with backslash.
I wonder why you didn't make an explicit list of the escaped
single-characters,
POST_EVENT_CHAR [][|)(~] POST_EVENT_COMMAND \\[][)(><~!\\] so you catch errors like \= as an unexpected '=' rather than undefined
command
"=".
Why would I want to do that when part of the motivation is to make the user able to define \= ? It makes more sense to define those as _inverted_ sets, namely _exclude_ every character that the parser needs to see as a separate token class. For \x, this is more or less the case implicitly. For straightforward non-letter characters such as @, this code is not really yet doing that. You ask why the \x set is not closed down. I'd rather go into the direction of opening up the x set... https://codereview.appspot.com/12432043/diff/6001/python/convertrules.py File python/convertrules.py (right): https://codereview.appspot.com/12432043/diff/6001/python/convertrules.py#newcode3614 python/convertrules.py:3614: str = re.sub (r"([-^_])\||" + matchstring + r"|[-^_][-^_]", subnonstring, str) On 2013/08/05 19:21:45, Keith wrote:
convert-ly rules are unlikely to be maintained, but this one is so
non-obvious
that you might still want a comment: # Match strings, and articulations that end in -^_ # So that we avoid leave alone -| in quoted strings and c4--|
Can do (though I'll reword the second line). https://codereview.appspot.com/12432043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
