[Fwd: Tracker 836:]

[ian_hulin]

I managed to get set up and put the patch for Tracker 836 on as tracker
141076.

However it looks like I had a bad mail address for this list in my
git-cl configuration.

Cheers,

Ian

Reviewers: ,

Message:
ly/engraver-init.ly in not part of this patch and I didn't change it.



http://codereview.appspot.com/141076/diff/2001/2003
File ly/engraver-init.ly (left):

http://codereview.appspot.com/141076/diff/2001/2003#oldcode353
Line 353: \consists "Bar_engraver"
This file and this change are not part of the changes in the patch-set
on my local repo.
Ian Hulin.

http://codereview.appspot.com/141076/diff/2001/2006
File scm/lily-library.scm (right):

http://codereview.appspot.com/141076/diff/2001/2006#newcode144
Line 144: (ly:parser-lookup parser 'book-filename))))
Coding
(book-filename))))
here using the local variable caused an "invalid type" error from Scheme

http://codereview.appspot.com/141076/diff/2001/2006#newcode151
Line 151: (ly:parser-lookup parser 'book-output-suffix)
Coding
(book-output-suffix)
here for the the clause of the if statement - using the local variable -
caused an "invalid type" error from Scheme.

http://codereview.appspot.com/141076/diff/2001/2006#newcode154
Line 154: (define-public current-outfile-name #f)  ; for use by
regression tests
current-outfile-name was added to help write a regression test for this
patch.

Description:
Tracker 836:

   Add facility to change output file-name for a \book block or to set a
   suffix to prevent multiple files over-writing each other during a
   compilation.
   This change allows user to to this via functions rather than having to
do so
   so by manipulating semi-documented parser variables.

Merge branch 'master' of git://git.sv.gnu.org/lilypond


Tracker 836:

   Add facility to change output file-name for a \book block or to set a
   suffix to prevent multiple files over-writing each other during a
   compilation.
   This change allows user to to this via functions rather than having to
do so
   so by manipulating semi-documented parser variables.

[PATCH] Add missing documentation strings in property-init.ly and

  music-functions-init.ly.

Please review this at http://codereview.appspot.com/141076

Affected files:
   M lily/parser.yy
   M ly/engraver-init.ly
   M ly/init.ly
   M ly/music-functions-init.ly
   M scm/lily-library.scm



______________________________________________        
This email has been scanned by Netintelligence        
http://www.netintelligence.com/email



_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Neil Puttock]

2009/10/28 Ian Hulin <ian@...>:
> I managed to get set up and put the patch for Tracker 836 on as tracker
> 141076.

This issue doesn't exist; did you delete it after uploading?

> Message:
> ly/engraver-init.ly in not part of this patch and I didn't change it.

You should apply your patch to master, then you won't get previous
commits showing up.

The way I normally do it is as follows:

-) create a patch which you want to upload
-) reset master branch (if necessary, pull any changes)
-) create a new branch for uploading to Rietveld
-) checkout new branch and apply patch
-) upload using `git cl upload origin'

> http://codereview.appspot.com/141076/diff/2001/2006#newcode144
> Line 144: (ly:parser-lookup parser 'book-filename))))
> Coding
> (book-filename))))
> here using the local variable caused an "invalid type" error from Scheme

Please read my suggestion again (and Carl's explanation).

> http://codereview.appspot.com/141076/diff/2001/2006#newcode154
> Line 154: (define-public current-outfile-name #f)  ; for use by
> regression tests
> current-outfile-name was added to help write a regression test for this
> patch.

Don't keep us in suspense then; add the regression test to the patch
so we can review it. :)

There's one more bug with output-count.  I suspect this line

(assoc-set! counter-alist output-suffix (1+ output-count)))

needs tweaking:

(assoc-set! counter-alist alist-key (1+ output-count)))

Cheers,
Neil


_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Carl Sorensen-3]

On 10/28/09 4:39 PM, "Neil Puttock" <n.puttock@...> wrote:

> 2009/10/28 Ian Hulin <ian@...>:
>> I managed to get set up and put the patch for Tracker 836 on as tracker
>> 141076.
>
> This issue doesn't exist; did you delete it after uploading?
>
>> Message:
>> ly/engraver-init.ly in not part of this patch and I didn't change it.
>
> You should apply your patch to master, then you won't get previous
> commits showing up.
>
> The way I normally do it is as follows:
>
> -) create a patch which you want to upload
> -) reset master branch (if necessary, pull any changes)
> -) create a new branch for uploading to Rietveld
> -) checkout new branch and apply patch
> -) upload using `git cl upload origin'
>
>> http://codereview.appspot.com/141076/diff/2001/2006#newcode144
>> Line 144: (ly:parser-lookup parser 'book-filename))))
>> Coding
>> (book-filename))))
>> here using the local variable caused an "invalid type" error from Scheme
>
> Please read my suggestion again (and Carl's explanation).

Neil's suggestion did *not* have a parenthesis before the book-filename

(book-filename) tries to evaluate a procedure book-filename and return the
results.  But book-filename isn't a procedure; hence the invalid type.

book-filename just returns the value of the variable book-filename.

Unlike in most languages, in  Scheme you can't insert unnecessary
parentheses in expressions; every parenthesis is significant.

HTH,

Carl



_______________________________________________
lilypond-devel mailing list
lilypond-devel@...
http://lists.gnu.org/mailman/listinfo/lilypond-devel