Code review - Issue 5322065: Implement \once as music function able to operate on complex stuff.https://codereview.appspot.com/2011-11-03T23:42:09+00:00rietveld
Message from unknown
2011-11-01T17:52:02+00:00dakurn:md5:1af7f4f3e6d19fa92c05e66481af9844
Message from reinhold.kainhofer@gmail.com
2011-11-03T10:47:59+00:00Reinholdurn:md5:fcf1c14ab186d94d14751eeb3ce47a3a
LGTM, although regtests are missing (for \once applied to multiple settings at one, stored in a variable).
Message from unknown
2011-11-03T11:23:44+00:00dakurn:md5:d53b2d65bb7d7c63d2b42a52798327dd
Message from dak@gnu.org
2011-11-03T11:25:49+00:00dakurn:md5:579f059e85e6eb934fd59b1f360b47e2
On 2011/11/03 10:47:59, Reinhold wrote:
> LGTM, although regtests are missing (for \once applied to multiple settings at
> one, stored in a variable).
There you are.
Message from reinhold.kainhofer@gmail.com
2011-11-03T11:45:00+00:00Reinholdurn:md5:d5192d6d90188271956c51e9045e953b
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly
File input/regression/complex-once.ly (right):
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode11
input/regression/complex-once.ly:11: \unHideNotes g a \once\hideNotes b c |
While this is the easy approach, it builds on the fact that hideNotes is internally implemented as a music expression with several \override commands. If that internal representation ever changes, we won't have a proper regtest any more...
I would prefer to define the multiple property operations command explicitly in the regtest, so we have complete control and no risk that a totally unrelated change might break a regtest (without us even noticing).
I.e. I would explicitly do
multipleOperations = {
\override #'NoteHead #'color = #red
\override #'Stem #'color = #blue
}
and then \once\multipleOperations.
Message from dak@gnu.org
2011-11-03T12:57:26+00:00dakurn:md5:39e647882a442e67c30cfd9768cb51c1
On 2011/11/03 11:45:00, Reinhold wrote:
> http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly
> File input/regression/complex-once.ly (right):
>
> http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode11
> input/regression/complex-once.ly:11: \unHideNotes g a \once\hideNotes b c |
> While this is the easy approach, it builds on the fact that hideNotes is
> internally implemented as a music expression with several \override commands. If
> that internal representation ever changes, we won't have a proper regtest any
> more...
> I would prefer to define the multiple property operations command explicitly in
> the regtest, so we have complete control and no risk that a totally unrelated
> change might break a regtest (without us even noticing).
>
> I.e. I would explicitly do
> multipleOperations = {
> \override #'NoteHead #'color = #red
> \override #'Stem #'color = #blue
> }
>
> and then \once\multipleOperations.
I disagree. The whole point of redefining \once is to make it useful for _existing_ functions/variables known to work by meddling with properties.
Regtests also serve as illustrations of functionality. Defining an explicit sandbox for the regtest to work on is completely eliminating its educational value.
I consider it very unlikely that changing \hideNotes in a manner where it would either
a) work with the previous definition of \once
b) keep working with the new definition of \once while failing to work
with a sandbox like the above
is feasible at all.
So I consider the regtest as _fully_ doing its intended job. The whole point of the change is that you can use \once on existing commands like \hideNotes. That's the desired and intended behavior.
Message from dak@gnu.org
2011-11-03T14:15:33+00:00dakurn:md5:2b13cb9bb82073c0f5756f6ebd5f8f4d
As an illustration: take a look at ly/property-init.ly. It contains dozens of commands that will cooperate with the new \once. All of them (well, the reverting commands don't, just because \once\revert is not [yet?] workable in general). That's its principal selling point.
And I don't want to hide that. It's much more important than testing that it will work with some custom-defined command.
Message from n.puttock@gmail.com
2011-11-03T14:21:05+00:00Neil Puttockurn:md5:0e8aacdba2dea382ee7546db6dc45981
LGTM.
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly
File input/regression/complex-once.ly (right):
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode7
input/regression/complex-once.ly:7: \layout { ragged-right = ##t }
redundant
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode11
input/regression/complex-once.ly:11: \unHideNotes g a \once\hideNotes b c |
remove extra spaces
\once \hideNotes
http://codereview.appspot.com/5322065/diff/3001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):
http://codereview.appspot.com/5322065/diff/3001/ly/music-functions-init.ly#newcode583
ly/music-functions-init.ly:583: (_i "Set @code{once} to @code{#t} on all layout instruction events in @var{music}")
add full stop/period
http://codereview.appspot.com/5322065/diff/3001/ly/music-functions-init.ly#newcode589
ly/music-functions-init.ly:589: (ly:music-warning m (_ "Can't apply \\once to timed music"))))
cannot (or can not, whatever we use)
Message from dak@gnu.org
2011-11-03T14:47:54+00:00dakurn:md5:ed7e30e5f031a49204b29b5e04e66566
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly
File input/regression/complex-once.ly (right):
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode7
input/regression/complex-once.ly:7: \layout { ragged-right = ##t }
On 2011/11/03 14:21:05, Neil Puttock wrote:
> redundant
I remembered reading somewhere that every test should have this. No idea why. No idea where. Can't find it in CG, no idea where else. Probably copied it from some existing test. Will remove.
http://codereview.appspot.com/5322065/diff/3001/input/regression/complex-once.ly#newcode11
input/regression/complex-once.ly:11: \unHideNotes g a \once\hideNotes b c |
On 2011/11/03 14:21:05, Neil Puttock wrote:
> remove extra spaces
>
> \once \hideNotes
Will do.
Message from unknown
2011-11-03T14:59:59+00:00dakurn:md5:a0b2ee5cfb06e02f7dfd66cb92e7c20b
Message from pkx166h@gmail.com
2011-11-03T23:42:09+00:00pkx166hurn:md5:ea614ed193241c3d857f6c08b8f50fec
Passes make and no reg test diffs.
James