| Re: [frogs] [PATCH]: Tracker 836 - Allow output filename and output-suffix to be specified for a \book block |
[ Thread Index |
Date Index
| More lilynet.net/frogs Archives
]
Carl Sorensen wrote:
On 10/27/09 7:05 PM, "Neil Puttock" <n.puttock@xxxxxxxxx> wrote:
2009/10/27 Carl Sorensen <c_sorensen@xxxxxxx>:
The patch looks good to me. Neil?
Not bad, Ian, but you need to keep an eye on trailing spaces and indentation.
To try to save you time, I'm going to explain the indentation issues.
+ PARSER->lexer_->set_identifier (ly_symbol2scm
("book-output-suffix"), SCM_BOOL_F);
+ PARSER->lexer_->set_identifier (ly_symbol2scm
("book-filename"), SCM_BOOL_F);
indentation
These lines are indented one column relative to the previous line.
Sorted. Needed to set stuff indents and tabs properly in JEdit.
+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
+@var{newsuffix}")
+ (set! book-output-suffix newsuffix)
+ (make-music 'SequentialMusic 'void #t))
+
(set! and (make-music should not be indented relative to the docstring.
Also, the docstring is probably indented too far.
+ %% why a function?
indentation
Sorted, same cause as above.
+ (make-music 'SequentialMusic 'void #t))
You could add a `make-void-music' function for this as it's used in
quite a few places.
If you choose to do this, there should also be a convert-ly rule added to
make this substitution in the docs, regtests, input files, and snippets.
I'm trying to keep the scope of this change to what I need to sort out
the filenames and suffixes. The only change I made was to
addInstrumentName once I'd tested the function with bookOutputName and
bookOutputSuffix so I could beat git into submission to recognize a
changed file.
+;; 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).
Done
+(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
(let* should be aligned with the "d" or the "e" in "define"
(if should be aligned with the "l" or the "e" in let
(book-filename should not begin on a new line
(ly:parser-output and (ly:parser-lookup should be aligned with (not
+(define (get-current-filename parser)
+ (let* (
+ (book-filename (ly:parser-lookup parser 'book-filename)))
(let (book-filename (ly:parser-lookup parser 'book-filename)))
Only use let* if you have multiple variable declarations in the let, and if
later declarations need to use the result of an earlier declaration.
Otherwise use let. And keep the first declaration on the same line as the
let.
+ (ly:parser-lookup parser 'book-filename))))
book-filename)))
You already had the result of (ly:parser-lookup parser 'book-filename),
so there's no need to do the call again.
+(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
Done for both functions. Thanks for pointing out about the brackets, Carl.
+(define-public current-outfile-name #f) ; for use by regression tests
I don't understand this; have you added it for use later?
Yes, I'm working on a regtest for this and T714. I want to be able to
test the output from print-book-with without having to generate backend
files.
+(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
first declaration on the same line as the let*, all others indented to
match.
In Scheme we *never* end a line with an open parenthesis.
A good discussion of Scheme indenting is found at
<http://community.schemewiki.org/?scheme-style>
+ (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)))
Sorted as follows:
(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))
;; Allow all ASCII alphanumerics, including accents
(if (string? output-suffix)
(set! result (format "~a-~a"
result (string-regexp-substitute
"[^-[:alnum:]]" "_" output-suffix))))
;; assoc-get call will always have returned a number
(if (> output-count 0)
(set! result (format #f "~a-~a" result output-count)))
BTW, have you installed git-cl yet? It would be much easier to review
your patches using Rietveld.
Now that you're up and going with patches, it would be helpful to have you
use Rietveld. Instructions for using git-cl are in the Contributors Guide,
section 8.7.9 (under Adding or modifying features).
Sorry for all the false starts, I've now managed to get a clean patch
containing only my changed files onto rietveld as Issue 143055.
Thanks for your patience and help on this one.
Cheers,
Ian
---
----
Join the Frogs!