Re: [frogs] [PATCH]: Tracker 836 - Allow output filename and output-suffix to be specified for a \book block

[Neil Puttock]

2009/10/27 Carl Sorensen <address@hidden>:

> The patch looks good to me.  Neil?

Not bad, Ian, but you need to keep an eye on trailing spaces and indentation.

+        PARSER->lexer_->set_identifier (ly_symbol2scm
("book-output-suffix"), SCM_BOOL_F);
+        PARSER->lexer_->set_identifier (ly_symbol2scm
("book-filename"), SCM_BOOL_F);

indentation

+bookOutputName =
+#(define-music-function (parser location newfilename) (string?)
+    (_i "Direct output for the current book block to @var{newfilename}")
+        (set! book-filename newfilename)
+        (make-music 'SequentialMusic 'void #t))
+
+bookOutputSuffix =
+#(define-music-function (parser location newsuffix) (string?)
+    (_i "Set the output filename suffix for the current book block to
address@hidden")
+        (set! book-output-suffix newsuffix)
+        (make-music 'SequentialMusic 'void #t))
+
+        %% why a function?

indentation

+        (make-music 'SequentialMusic 'void #t))

You could add a `make-void-music' function for this as it's used in
quite a few places.

+;; return any suffix value for output filename allowing for settings by
+;; calls to \bookOutputName

I'd document this inside the function as a string (ditto for
get-current-suffix).

+(define (get-current-filename parser)
+        (let* (
+                (book-filename (ly:parser-lookup parser 'book-filename)))
+            (if (not book-filename)
+            (ly:parser-output-name parser)
+            (ly:parser-lookup parser 'book-filename))))

indentation

+(define (get-current-filename parser)
+        (let* (
+                (book-filename (ly:parser-lookup parser 'book-filename)))

(let (book-filename (ly:parser-lookup parser 'book-filename)))

+            (ly:parser-lookup parser 'book-filename))))

book-filename)))

+(define (get-current-suffix parser)
+   (let* (
+            (book-output-suffix (ly:parser-lookup parser 'book-output-suffix)))
+    (if (string? book-output-suffix)
+        (ly:parser-lookup parser 'book-output-suffix)
+        (ly:parser-lookup parser 'output-suffix))))

indentation

+   (let* (
+            (book-output-suffix (ly:parser-lookup parser 'book-output-suffix)))

(let (book-output-suffix (ly:parser-lookup parser 'book-output-suffix)))

+        (ly:parser-lookup parser 'book-output-suffix)

book-output-suffix

+(define-public current-outfile-name #f)  ; for use by regression tests

I don't understand this; have you added it for use later?

+(define (get-outfile-name parser)
+    ;; user can now override the base file name, so we have to use
+    ;; the file-name concatenated with any potential output-suffix value
+    ;; as the key to out internal a-list
+  (let* (
+      (base-name (get-current-filename parser))
+      (output-suffix (get-current-suffix parser))
+      (alist-key (format "~a~a" base-name output-suffix))
+      (counter-alist (ly:parser-lookup parser 'counter-alist))
+      (output-count (assoc-get alist-key counter-alist 0))
+      (result base-name))

indentation

+  (let* (
+      (base-name (get-current-filename parser))

(let* ((base-name (get-current-filename parser))

The patch doesn't work due to the following gremlins:

                             base (string-regexp-substitute

base-name (string-regexp-substitute

         (outfile-name (get-outfile-name parser base)))

(outfile-name (get-outfile-name parser)))

BTW, have you installed git-cl yet?  It would be much easier to review
your patches using Rietveld.

Regards,
Neil