carl.d.soren...@gmail.com writes: > David, > > Thank you for posting this on Rietveld. It was much easier for me to > review it. > > I have one documentation question (see below). > > I also have one patch philosophy question: should define-markup also > have (at least) a :properties keyword added to it?
Goal #1 was to be able to move user-level markup to system markup unchanged. This goal actually is not achieved with the current state of the patch series since it fails for markup definitions without DOC string. > I think that if it did, then we'd have the possibility of using the > exact same code and just changing from define-markup to > define-builtin-markup by only changing the macro name. After fixing the DOC string issue and exporting define-builtin-markup-command as define-markup-command, the regression test seems to work fine (namely, it runs until it crashes on "Unable to load the map file" which it always does here) and the scoping would appear to be correct as well. Which suggests that the apparent main reason for *-builtin-* commands, the changed toplevel scoping, is not valid, and the scoping works out well enough without extra complications. Renaming define-builtin-markup-command to define-markup-command, making it public and adapting all callers would appear to finish the job, obviously unifying the syntax. I have to check that this is indeed the case. > http://codereview.appspot.com/160048/diff/1/5 > File scm/markup.scm (right): > > http://codereview.appspot.com/160048/diff/1/5#newcode74 > scm/markup.scm:74: [ :category category ] > Does this need to be [ #:category category ] ? Yes. Sorry. -- David Kastrup _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
