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

[Ian Hulin]

Carl Sorensen wrote:
> 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.

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

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