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

[ianhulin44]

Remove debugging cruft, also other changes in patch-set one not needed
since patchset two.

Add handler to handle conditions thrown from markup module code in
lily.scm.


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))
On 2011/12/21 18:32:50, dak wrote:
> Is this required?
Not with latest patch-set, will remove

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)
On 2011/12/21 18:32:50, dak wrote:
> 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.
Reverted and removed corresponding changes in markup-facility-defs.scm

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)) )
On 2011/12/21 18:32:50, dak wrote:
> 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?
Left-over cruft from debugging.  Removing.

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.
On 2011/12/21 18:32:50, dak wrote:
> What's with the x?
x est res icognita. . .
OK it's a typo, will remove.

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm
File scm/lily.scm (right):

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm#newcode829
scm/lily.scm:829: (set! failed (append (list failed-file) failed)))))
Needs to be modified to handle conditions thrown from markup module.
(See also comment below)

http://codereview.appspot.com/5464045/diff/8002/scm/lily.scm#newcode873
scm/lily.scm:873: (lambda (x . args) (handler x file-name))))
Enhance to handle conditions thrown from markup module, also
handler assignment above in (let* block in lilypond-all

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#newcode17
scm/markup-facility-defs.scm:17: (define-public lilypond-module
(current-module))
To be removed, along with mirroring line at the bottom.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode34
scm/markup-facility-defs.scm:34: markup* )
On 2011/12/21 18:32:50, dak wrote:
> markup*?
It's cruft. Will lose it.

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."
On 2011/12/21 18:32:50, dak wrote:
> Why are you reintroducing markup*?
It's cruft. Will lose it.

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))
On 2011/12/21 18:32:50, dak wrote:
> Is there a reason to insert this debug output including the current
module?
Would like to leave this and similar until patch for Issue 1686 is done.

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))
On 2011/12/21 18:32:50, dak wrote:
> Same here.
See above

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode392
scm/markup-facility-defs.scm:392: (defmacro*-public markup* (#:rest
body)
On 2011/12/21 18:32:50, dak wrote:
> Again: is there a reason you reintroduce markup*?
It's cruft. Will lose it.

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
On 2011/12/21 18:32:50, dak wrote:
> Is there a reason you keep this line in after commenting it out?  It
does not
> seem to convey useful information.
It's a TODO for me - will comment it as such, see matching comment
below.

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))
On 2011/12/21 18:32:50, dak wrote:
> You throw a signal here that I don't see being caught anywhere.
Thinko.  It's caught in the version of lily.scm I was using on my Guile
V2 system. Will ensure relevant handling code is added to ly:load in
lily.scm in next patch-set.

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode458
scm/markup-facility-defs.scm:458: ;;)   ; Guile V2 only
On 2011/12/21 18:32:50, dak wrote:
> What is Guile V2 only here?
Part of the same TODO as at the top.  Going to have to do something
like:
(define (compile-markup-expression-aux <current-args>)
  <current compile-markup-expression code>)

(define (compile-markup-expression <current args>)
  (cond-expand
     (guile-2
        (eval-when (compile load eval)
           (compile-markup-expression-aux <current-args>)))
     (guile
           (compile-markup-expression-aux <current-args>)))

http://codereview.appspot.com/5464045/diff/8002/scm/markup-facility-defs.scm#newcode690
scm/markup-facility-defs.scm:690: (set-current-module lilypond-module)
On 2011/12/21 18:32:50, dak wrote:
> Is this line necessary?  What happens with it when loading, what when
compiling?
> Makes me nervous.
It was transitional while the file was still being loaded by lily.scm.
Should be redundant now lily.scm loads this via (use-modules) will
remove this and matching line 17.

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