I like what you've done. I've put a couple of comments in. They are not mandatory, but just for your consideration.
Carl http://codereview.appspot.com/115065/diff/1001/1003 File scm/paper.scm (right): http://codereview.appspot.com/115065/diff/1001/1003#newcode220 Line 220: (scaleable-values (list I think it would be better to use symbols, rather than strings, here. I don't have a strong preference for this; obviously these strings are only used locally to define symbols later on. But the things they stand for are symbols elsewhere in the code, so I'd prefer using symbols here. http://codereview.appspot.com/115065/diff/1001/1003#newcode221 Line 221: (cons "left-margin" w) I think you could alternatively do (either with symbols or strings): (scaleable-values `((left-margin . ,w) (right-margin . ,w) (top-margin . ,h) ... (short-indent . ,w)) http://codereview.appspot.com/115065/diff/1001/1003#newcode232 Line 232: ((value-symbol (string->symbol (string-append (car value) "-default"))) I you were using symbols, you'd have (value-symbol (string->symbol (string-append (symbol->string (car value)) "-default"))) http://codereview.appspot.com/115065/diff/1001/1003#newcode244 Line 244: (module-define! m 'left-margin-default-scaled (cdr (assoc "left-margin" scaled-values))) This is a potential error waiting to cause problems for a user. assoc can return #f, and (cdr #f) is an error. If you use assoc-get, you can supply a default value, e.g. (module-define! m 'left-margin-default-scaled (assoc-get 'left-margin scaled-values default-value)) where default-value is whatever the reasonable default value would be if things somehow got broken. I realize that you have just built the list here, so you're in control and it can be argued that this is not necessary. I'm not demanding this change, just asking you to consider it. http://codereview.appspot.com/115065 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
