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

[Carl Sorensen]

On 10/27/09 7:05 PM, "Neil Puttock" <address@hidden> wrote:

> 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.

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.

> 
> +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))
> +

(set! and (make-music should not be indented relative to the docstring.
Also, the docstring is probably indented too far.

> +        %% 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.

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.

> 
> +;; 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

(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
> 
> +(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

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)))
> 
> 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).

HTH,

Carl