Re: T2026: Use new (scm markup-facility-defs) scheme module for markup commands. (issue 5464045)

[dak]

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cyclic-reference.ly
File input/regression/markup-cyclic-reference.ly (right):

http://codereview.appspot.com/5464045/diff/8002/input/regression/markup-cyclic-reference.ly#newcode2
input/regression/markup-cyclic-reference.ly:2: #(use-modules (scm
markup-facility-defs))
Is this required?

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly
File ly/toc-init.ly (right):

http://codereview.appspot.com/5464045/diff/8002/ly/toc-init.ly#newcode4
ly/toc-init.ly:4: %% scheme code now defined in (scm
markup-facility-defs)
Can you explain why it was necessary to move the stuff in here to
markup-facility-defs?  It looks like the kind of thing that should
continue working.

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm
File scm/define-event-classes.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-event-classes.scm#newcode22
scm/define-event-classes.scm:22: (if (not (defined?
'ly:is-listened-event-class)) (use-modules (lily)) )
Uh, why the conditional here?  Isn't it normal to use-modules whenever
one needs something defined in a different module, and let use-modules
cater for loading the stuff if not already done?

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/define-markup-commands.scm#newcode370
scm/define-markup-commands.scm:370: x    laboris nisi ut aliquip ex ea
commodo consequat.
What's with the x?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm
File scm/markup-facility-defs.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode34
scm/markup-facility-defs.scm:34: markup* )
markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode94
scm/markup-facility-defs.scm:94: Use `markup*' in a \\notemode context."
Why are you reintroducing markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode169
scm/markup-facility-defs.scm:169: (ly:debug "Defined ~s function in
~s\n" ,(symbol->string command-name) (current-module))
Is there a reason to insert this debug output including the current
module?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode198
scm/markup-facility-defs.scm:198: (ly:debug "Defined ~s function in
~s\n" ,(symbol->string make-markup-name) (current-module))
Same here.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode392
scm/markup-facility-defs.scm:392: (defmacro*-public markup* (#:rest
body)
Again: is there a reason you reintroduce markup*?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode413
scm/markup-facility-defs.scm:413: ;; (eval-when (load compile eval) ;
Guile V2 only
Is there a reason you keep this line in after commenting it out?  It
does not seem to convey useful information.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode424
scm/markup-facility-defs.scm:424: (ly:message "Current module: ~s"
(current-module))
You throw a signal here that I don't see being caught anywhere.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode458
scm/markup-facility-defs.scm:458: ;;)   ; Guile V2 only
What is Guile V2 only here?

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode690
scm/markup-facility-defs.scm:690: (set-current-module lilypond-module)
Is this line necessary?  What happens with it when loading, what when
compiling? Makes me nervous.

http://codereview.appspot.com/5464045/