Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5257)

Issue 284060043: NR 3.3.2: nitpicks in lily example

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by simon.albrecht
Modified:
8 years, 2 months ago
Reviewers:
thomasmorley651
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

NR 3.3.2: nitpicks in lily example add a missing space and use a layout block instead of duplicating code

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -3 lines) Patch
M Documentation/notation/input.itely View 2 chunks +7 lines, -3 lines 3 comments Download

Messages

Total messages: 2
simon.albrecht
Please review.
8 years, 2 months ago (2016-01-03 21:59:26 UTC) #1
thomasmorley651
8 years, 2 months ago (2016-01-03 22:36:56 UTC) #2
Thanks for doing it. 
Some comments, though.

https://codereview.appspot.com/284060043/diff/1/Documentation/notation/input....
File Documentation/notation/input.itely (right):

https://codereview.appspot.com/284060043/diff/1/Documentation/notation/input....
Documentation/notation/input.itely:2147: \partcombine
While you're on it. partcombine is the function. sopranorMusic and altoMusic
their arguments. Thus I'd indent the arguments by two spaces.

https://codereview.appspot.com/284060043/diff/1/Documentation/notation/input....
Documentation/notation/input.itely:2153: \partcombine
same here

https://codereview.appspot.com/284060043/diff/1/Documentation/notation/input....
Documentation/notation/input.itely:2158: \layout {
Having it in layout is much better, yes, but here it makes no difference setting
it #t or #f or deleting it at all.
If we want to demonstrate it's effect, it's needed to extend the music-example.
If we want to provide sort of template, a comment should be inserted.
Or delete it at all.

Personally I've a slight preference for second option.
3rd option is possible as well, because we already explain printPartCombineTexts
in 

NR 1.5.2 Multiple voices 
         Automatic part combining	

http://www.lilypond.org/doc/v2.19/Documentation/notation/multiple-voices#auto...

The relevant snippet showing a nice example is:  	

Documentation/snippets/combining-two-parts-on-the-same-staff.ly
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b