Tracker 836: Add facility to change output file-name for a \book block

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Tracker 836: Add facility to change output file-name for a \book block

Ian Hulin
Reviewers: Neil Puttock, carl.d.sorensen_gmail.com,

Message:
The latest version of this patch is now ready for review.

Cheers,
Ian

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 do this via functions rather
than having to do so by using set! and define on parser variables in
Scheme.

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

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



---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
On 2009/11/08 20:17:37, Ian Hulin wrote:
> The latest version of this patch is now ready for review.

`warning: 15 lines add whitespace errors.'

There were twelve in the last patchset. :)

Five are space-before-tab issues, the rest trailing spaces.

Generally looks OK, apart from a few remaining indentation nitpicks in
\pitchedTrill.

Regards,
Neil

http://codereview.appspot.com/150044

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
In reply to this post by Ian Hulin

http://codereview.appspot.com/150044/diff/1/4
File ly/music-functions-init.ly (left):

http://codereview.appspot.com/150044/diff/1/4#oldcode604
ly/music-functions-init.ly:604: (ly:music-property main-note
'elements))))
with (lambda

http://codereview.appspot.com/150044/diff/1/4
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/150044/diff/1/4#newcode612
ly/music-functions-init.ly:612: (lambda (m) (eq? 'NoteEvent
(ly:music-property m 'name)))
with filter, not (filter

http://codereview.appspot.com/150044/diff/1/4#newcode613
ly/music-functions-init.ly:613: (ly:music-property ev-chord
'elements))))
as above

http://codereview.appspot.com/150044/diff/1/4#newcode620
ly/music-functions-init.ly:620: (let* ((trill-pitch (ly:music-property
(car sec-note-events) 'pitch))
with `e' in begin

http://codereview.appspot.com/150044/diff/1/4#newcode621
ly/music-functions-init.ly:621: (forced (ly:music-property (car
sec-note-events )
with (trill-pitch

http://codereview.appspot.com/150044/diff/1/4#newcode624
ly/music-functions-init.ly:624: (if (ly:pitch? trill-pitch)
with `e' in let*

http://codereview.appspot.com/150044/diff/1/4#newcode631
ly/music-functions-init.ly:631: (if (eq? forced #t)
as (if (ly:pitch? trill-pitch)

http://codereview.appspot.com/150044

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Ian Hulin
In reply to this post by Ian Hulin
Have also looked for traling spaces in lily-library.scm


http://codereview.appspot.com/150044/diff/1/4
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/150044/diff/1/4#newcode612
ly/music-functions-init.ly:612: (lambda (m) (eq? 'NoteEvent
(ly:music-property m 'name)))
On 2009/11/08 21:40:29, Neil Puttock wrote:
> with filter, not (filter

Done.

http://codereview.appspot.com/150044/diff/1/4#newcode620
ly/music-functions-init.ly:620: (let* ((trill-pitch (ly:music-property
(car sec-note-events) 'pitch))
On 2009/11/08 21:40:29, Neil Puttock wrote:
> with `e' in begin

Done.

http://codereview.appspot.com/150044/diff/1/4#newcode624
ly/music-functions-init.ly:624: (if (ly:pitch? trill-pitch)
On 2009/11/08 21:40:29, Neil Puttock wrote:
> with `e' in let*

Done.

http://codereview.appspot.com/150044/diff/1/4#newcode631
ly/music-functions-init.ly:631: (if (eq? forced #t)
On 2009/11/08 21:40:29, Neil Puttock wrote:
> as (if (ly:pitch? trill-pitch)

Done.

http://codereview.appspot.com/150044

---

----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Ian Hulin
In reply to this post by Ian Hulin
Hi all,
Is it O K to push this patch now?

If so, can either Carl or Neil push this on origin/master.

Cheers and thanks for all the help on this one,
Ian

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Carl.D.Sorensen
In reply to this post by Ian Hulin
A few whitespace errors (tab following spaces) and one indenting
mistake.  Then I think it's good to go.




http://codereview.appspot.com/150044/diff/11/1015
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/150044/diff/11/1015#newcode611
ly/music-functions-init.ly:611: (filter
Tab following space -- we never want to have that.  Start the line with
as many tabs as desired, followed by spaces if necessary to create the
proper indentation.  Or use all spaces -- that's fine too.  But not
space tab space.

http://codereview.appspot.com/150044/diff/11/1015#newcode612
ly/music-functions-init.ly:612: (lambda (m) (eq? 'NoteEvent
(ly:music-property m 'name)))
Tab following space

http://codereview.appspot.com/150044/diff/11/1015#newcode613
ly/music-functions-init.ly:613: (ly:music-property ev-chord
'elements))))
Tab following space

http://codereview.appspot.com/150044/diff/11/1015#newcode624
ly/music-functions-init.ly:624: (for-each (lambda (m)
(for-each should align with (ly:pitch?.

(if (test-expression)
     (true-expression)
     (false-expression))

http://codereview.appspot.com/150044/diff/11/1015#newcode626
ly/music-functions-init.ly:626: (begin
After changing for-each, you'll need to change (begin to match.

http://codereview.appspot.com/150044/diff/11/1015#newcode633
ly/music-functions-init.ly:633: trill-events)))))
Tab following spaces

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
In reply to this post by Ian Hulin
On 2009/11/12 01:12:35, Carl wrote:
> A few whitespace errors (tab following spaces) and one indenting
mistake.  Then
> I think it's good to go.

A useful tip for catching these errors is to apply the patch locally:
git will tell you where the whitespace errors are.

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
In reply to this post by Ian Hulin

http://codereview.appspot.com/150044/diff/11/1015
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/150044/diff/11/1015#newcode621
ly/music-functions-init.ly:621: (forced (ly:music-property (car
sec-note-events ) 'force-accidental)))
(car sec-note-events)

http://codereview.appspot.com/150044/diff/11/1015#newcode623
ly/music-functions-init.ly:623: (if (ly:pitch? trill-pitch)
with `e' in (let* above

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
In reply to this post by Ian Hulin
On 2009/11/12 01:12:35, Carl wrote:
> A few whitespace errors (tab following spaces) and one indenting
mistake.  Then
> I think it's good to go.

A useful tip for catching these errors is to apply the patch locally:
git will tell you where the whitespace errors are.

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Ian Hulin
In reply to this post by Ian Hulin
music-functions-init.ly
  spaces+tab lines sorted.
  indentation ll 626-633 fixed.

Is the patch ready to push now?

Cheers,
Ian



http://codereview.appspot.com/150044/diff/11/1015
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/150044/diff/11/1015#newcode611
ly/music-functions-init.ly:611: (filter
On 2009/11/12 01:12:36, Carl wrote:
> Tab following space -- we never want to have that.  Start the line
with as many
> tabs as desired, followed by spaces if necessary to create the proper
> indentation.  Or use all spaces -- that's fine too.  But not space tab
space.

Done.

http://codereview.appspot.com/150044/diff/11/1015#newcode612
ly/music-functions-init.ly:612: (lambda (m) (eq? 'NoteEvent
(ly:music-property m 'name)))
On 2009/11/12 01:12:36, Carl wrote:
> Tab following space

Done.

http://codereview.appspot.com/150044/diff/11/1015#newcode613
ly/music-functions-init.ly:613: (ly:music-property ev-chord
'elements))))
On 2009/11/12 01:12:36, Carl wrote:
> Tab following space

Done.

http://codereview.appspot.com/150044/diff/11/1015#newcode626
ly/music-functions-init.ly:626: (begin
On 2009/11/12 01:12:36, Carl wrote:
> After changing for-each, you'll need to change (begin to match.

Done.

http://codereview.appspot.com/150044/diff/11/1015#newcode633
ly/music-functions-init.ly:633: trill-events)))))
On 2009/11/12 01:12:36, Carl wrote:
> Tab following spaces

Done.

http://codereview.appspot.com/150044

---
----
Join the Frogs!

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Tracker 836: Add facility to change output file-name for a \book block

Neil Puttock
2009/11/12  <[hidden email]>:

> Is the patch ready to push now?

LGTM, apart from a few remaining whitespaces and wonky indents. :)

I've sorted these and will push to master shortly.

Thanks for being so patient,
Neil

---
----
Join the Frogs!

Loading...