Re: quarter tones in tablature (issue 272320043 by address@hidden)

[thomasmorley65]

On 2015/11/08 14:22:26, dak wrote:
> I'm currently trying to get tax declarations finished with a very very
looming
> deadline, so I am worse at doing reviews properly than I am anyway.
This one
> hit me in the face though and would warrant changing before people
start relying
> on it.

Ok.
There's
https://sourceforge.net/p/testlilyissues/issues/3088/
currently on count-down, as well.
I could imagine you would like to say something to it.
Should I put it back on 'review' (or maybe 'needs-work')?


https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm
> File scm/translation-functions.scm (right):


https://codereview.appspot.com/272320043/diff/1/scm/translation-functions.scm#newcode237
> scm/translation-functions.scm:237: ((determine-frets #:optional
> (support-non-integer-fret? #f))
> That's pretty bad.  determine-frets was a public function previously,
changing
> it to a hook it to a function generator is incompatible.

I don't understand. Why incompatible? At least it survived our regtests
and my local testings.

> As a note aside, define*-public with curryied functions is broken in
Guile-2 and
> needs to get split into define* and export separately.  But that's not
really
> relevant here.

Ok, understood.

> support-non-integer-fret belongs into a context property, and
determine-fret
> gets passed a context anyway, so that's quite straightforward to do.
No need to
> change the call interface to determine-frets.

I could create a follow up, introducing "support-non-integer-fret?" as a
context-property and change determine-fret-code accordingly.
I think that's your suggestion.

https://codereview.appspot.com/272320043/