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

[Carl Sorensen]

On 10/29/09 8:01 AM, "Ian Hulin" <address@hidden> wrote:

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

Does Jedit handle Scheme properly?  One of the issues is that it's different
from .ly files.


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

Since Neil said "could", I assume you don't need to to get the patch
accepted.

But a follow-on patch would probably be appreciated.
>> 
>>> (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.
>

You're welcome.  Scheme can be very confusing at times, can't it.
 
> 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)))
> 

I still see some formatting issues:

   (let* ((base-name (get-current-filename parser))
          ^ eliminated space between parentheses -- not used in Scheme
          (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))))
               ^  (format is aligned with result, since it's at the same
                    logical level.
                       ^  result and (string... are aligned with "~a-~a"
                          since they're at the same logical level
                         ^ the args to string-regexp-substitute are all
                           indented equally relative to the opening paren
                           of the list.

     ;; assoc-get call will always have returned a number
     (if (> output-count 0)
         (set! result (format #f "~a-~a" result output-count)))
         ^ The resultant expresssion is aligned with the conditional
           expression. (Which is another way of saying the list elements are
           aligned.)

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

No need to apologize.  This can be a difficult system to get up to speed on.
You're doing really well.  Thanks for your patience and willingness to
learn.

> 
> Thanks for your patience and help on this one.

You're *very* welcome.  It's great to have new people joining in on LilyPond
development.

Carl