Re: Issue 3720: Built-in templates for SATB vocal scores (issue 41990043)

[tdanielsmusic]

On 2014/01/05 01:42:55, dak wrote:
> https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly
> File ly/satb.ly (right):

> https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode1
> ly/satb.ly:1: %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
> Missing \version number is causing an error when running convert-ly.


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode68
> ly/satb.ly:68: (if (defined? name) name (if (pair? default) (car
default)
> '#{#})))
> '#{#} is expensive.  I'd rather use *unspecified* here.


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode70
> ly/satb.ly:70: #(define (sym . strings) (string->symbol (apply
string-append
> strings)))
> For an include file (rather than a module), this and some other macro
names seem
> awfully likely to cause collisions.


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode84
> ly/satb.ly:84: (let ((above (if (pair? optionals) (car optionals)
#f)))
> (if x y #f) is easier to understand and write as (and x y), in
particular if x
> takes up considerable space.


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode93
> ly/satb.ly:93: #{#})))
> Why #{#} unquoted?  That evaluates at macro placement time.  But I'd
use
> *unspecified* anyway.


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode97
> ly/satb.ly:97: \new Staff = #(identity ,name) \with {
> Why #(identity ,name) here instead of #,name ?


https://codereview.appspot.com/41990043/diff/60001/ly/satb.ly#newcode104
> ly/satb.ly:104: \clef #(identity ,clef)
> What's with the identity?  It does not make sense.

> I'll skip the copious other occurences but they should be fixed.

As this patch has already been pushed I'll open a new
tracker to cover these issues.

Trevor


https://codereview.appspot.com/41990043/