Doc: LM: Reformat ly code.

Doc: LM: Reformat ly code.

* Except for Scheme code, add exactly two spaces (never
  tabs) for each additional level of indentation.
* Do not exceed one measure per line of code.
* Specify durations for the first note of every line.
* Specify durations after every `{' or `}'.
* Otherwise, specify durations only when they change.
* In \markup blocks, keep individual expressions on
  their own lines.
* Keep these commands on their own lines
  (and in this order):
    \clef
    \key
    \time
    \partial
* Use `\clef treble' instead of `\clef "treble"', etc.
* Use bar-checks (`|') only when barring is unclear.
* Do not put two `{'s on the same line unless the first
  `{' closes with a `}' before the second `{'
* Put a newline after `{' (and indent) if its '}' is not
  on the same line.
* Put a newline before `}' (and unindent) if its `{' is
  not on the same line.
* Put a single space before and after `{' and `}' unless
  it begins or ends a line.
* Put an empty line after the last \include.
* Surround with empty lines things like:
    #(set-global-staff-size x)
    #(ly:set-option x y)
* Put an empty line before and after multiline variable
  definitions or multiline blocks that start in the
  first column.
* For music function definitions:
  - put `functionName =' alone on the first line.
  - put `#(define-music-function' alone on the second.
  - indent `(parser location ...)' with 5 spaces.
  - indent `(type? ...)' with 5 spaces.
  - indent `#{' and `#}' with 3 spaces.
  - indent the contents of `#{ ... #}' with 5 spaces.
* For multiline variable definitions:
  - if `=' is immediately followed by `{', put a newline
    after `{'.
  - otherwise put a newline after `='.
* Always use curly braces with \markup.

[Graham Percival (old account), 2010-05-02 16:01:33]

I have many concerns about the expanded input code.  I also don't like seeing so
much formatting information in the patch comment... but since there isn't
agreement about all of it, maybe it's for the best that it isn't going in the CG
right now.

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode148
Documentation/learning/common-notation.itely:148: LilyPond makes a sharp
distinction between musical content and
Not related to your changes, but since I'm looking at it anyway, could you
change this "sharp" to "clear" ?  We don't want any confusion or bad puns
concerning "sharp" and accidentals.

http://codereview.appspot.com/1056041/diff/1/2#newcode155
Documentation/learning/common-notation.itely:155: rules.  The pitches in your
music are works of art, so they will
Another unrelated-to-your-changes: I don't follow the "works of art" comment. 
Could this be rewritten?

http://codereview.appspot.com/1056041/diff/1/2#newcode163
Documentation/learning/common-notation.itely:163: d4 cis fis
Existing doc policy is to use full bars unless there's a reason not to; could
you add some random note to the end?  (D, A, whatever)

http://codereview.appspot.com/1056041/diff/1/2#newcode177
Documentation/learning/common-notation.itely:177: b4
Maybe change this to:
c2 b
just to fill up the bar.

http://codereview.appspot.com/1056041/diff/1/2#newcode398
Documentation/learning/common-notation.itely:398: \dynamic f
Is the extra space necessary?  It adds 3 lines to the example.  We have
approximately 50 lines on each pdf page, so this increases the chance that the
example will be split over a page by 6%.

I mean, is the main point of this example to explain the details of how one
could write a forte-piano, or is it just to show that you can do fancier stuff
with text?  If we're just trying to show fancier text (which is what I think the
example is about), then I'd keep it a bit more condensed.

http://codereview.appspot.com/1056041/diff/1/2#newcode484
Documentation/learning/common-notation.itely:484: f8
I'm not certain that we want to split up
\partial 8 f8
(not that the previous example did that -- I'm just throwing a possible
alternate standard for dealing with partials)

http://codereview.appspot.com/1056041/diff/1/2#newcode719
Documentation/learning/common-notation.itely:719: \new Staff {
I agree with adding the 4 to c4.

I disagree with expanding this.  Is it honestly hard to understand
    \new Staff { \clef treble c4 }
?  By this point in Learning, the reader should be sufficiently familiar with
these commands that combining them onto a single line won't be a problem.

page-splitting probability of this change: +14%

http://codereview.appspot.com/1056041/diff/1/2#newcode755
Documentation/learning/common-notation.itely:755: \new Staff {
PSP: +14%.

I don't want to sound as though I want to condense everything; that's not the
case.  I just think that we should examine each case -- the point of this
example is nesting music expressions.  Expanding it doesn't increase
readability, the \new Staff is nothing new (no pun intended), so let's keep it
smaller to avoid page flipping or excessive scrolling.  We want people to be
able to grasp the whole example at a glance.

(regardless of the expansion, I definitely agree that we should have c4 there)

http://codereview.appspot.com/1056041/diff/1/2#newcode807
Documentation/learning/common-notation.itely:807: \new Staff {
c4 good, PSP +12% not worth the expansion.

http://codereview.appspot.com/1056041/diff/1/3
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/1/3#newcode447
Documentation/learning/fundamental.itely:447: } {
Given that we have
{ f8 c c }
above this, I don't think that
{ f8 f c }
is hard to understand.

http://codereview.appspot.com/1056041/diff/1/3#newcode726
Documentation/learning/fundamental.itely:726: { c8 b16 a b8 g~ g2 }
I personally prefer having a tie between to notes
g ~ g
rather than attached to the previous note.  But I'm not going to insist on that.
 Go ahead an make this change, but when GLISS happens, we're not necessarily
sticking to g~ g.

http://codereview.appspot.com/1056041/diff/1/3#newcode1052
Documentation/learning/fundamental.itely:1052: {
PSP +22% for the next three examples in total.  Is it really such a bad thing to
have multiple bars on the same line?  The rhythms are simple, the pitches are
simple, there's barline checks... I don't see what problem this is trying to
fix.

http://codereview.appspot.com/1056041/diff/1/3#newcode1261
Documentation/learning/fundamental.itely:1261: global = {
PSP +30% or 40% or something high.  Was the original example difficult to
understand?  It looks pretty simple to me, especially this far into Learning.

http://codereview.appspot.com/1056041/diff/1/3#newcode1333
Documentation/learning/fundamental.itely:1333: KeyTime = {
PSP +58%.  This case is totally not worth it.  The original example was trivial
to read.

http://codereview.appspot.com/1056041/diff/1/3#newcode1369
Documentation/learning/fundamental.itely:1369: }
If we have barline checks for the first few bars, please leave the final barline
check on the line.

http://codereview.appspot.com/1056041/diff/1/3#newcode2098
Documentation/learning/fundamental.itely:2098: c4 d
Why not make these
c2 d
to keep the one-bar-per-line idea?

http://codereview.appspot.com/1056041/diff/1/3#newcode2269
Documentation/learning/fundamental.itely:2269: c4 d
Ditto; make them half notes.

http://codereview.appspot.com/1056041/diff/1/3#newcode2537
Documentation/learning/fundamental.itely:2537: <<
Indentation mistake!

http://codereview.appspot.com/1056041/diff/1/3#newcode2611
Documentation/learning/fundamental.itely:2611: global = {
PSP = something, I can't see this being worth it.

http://codereview.appspot.com/1056041/diff/1/3#newcode3097
Documentation/learning/fundamental.itely:3097: 
My eyes would prefer \keyTime.  I don't mind you making this change, as long as
it's understood that we might standardize on camelcase during GLISS and thus
would need to change.

http://codereview.appspot.com/1056041/diff/1/3#newcode3153
Documentation/learning/fundamental.itely:3153: 
Was the original example difficult to understand without having spaces between
every four-line "stanza" ?

http://codereview.appspot.com/1056041/diff/1/3#newcode3245
Documentation/learning/fundamental.itely:3245: @c TODO Avoid padtext - not
needed with skylining
Dude!  If you're going to all this trouble to format stuff, and see a TODO like
this, couldn't you do something about it?

http://codereview.appspot.com/1056041/diff/1/3#newcode3312
Documentation/learning/fundamental.itely:3312: #(define-music-function
I like this spacing expansion.

http://codereview.appspot.com/1056041/diff/1/4
File Documentation/learning/tutorial.itely (right):

http://codereview.appspot.com/1056041/diff/1/4#newcode81
Documentation/learning/tutorial.itely:81: will produce an error message.
Please don't confuse the issue here; leave these c / C alone (without the 4). 
The focus is the case sensitivity.

http://codereview.appspot.com/1056041/diff/1/4#newcode174
Documentation/learning/tutorial.itely:174: c'4 e' g' e'
Please don't add the 4 to this example.

http://codereview.appspot.com/1056041/diff/1/4#newcode212
Documentation/learning/tutorial.itely:212: c4 d e f
Again, no durations yet; they haven't been introduced.

http://codereview.appspot.com/1056041/diff/1/4#newcode225
Documentation/learning/tutorial.itely:225: d4 f a g
Durations not introduced yet.

http://codereview.appspot.com/1056041/diff/1/4#newcode243
Documentation/learning/tutorial.itely:243: e4 c a c
No durs yet.

http://codereview.appspot.com/1056041/diff/1/4#newcode257
Documentation/learning/tutorial.itely:257: b4 c  % c is 1 staff space up, so is
the c above
No durs yet.

http://codereview.appspot.com/1056041/diff/1/4#newcode279
Documentation/learning/tutorial.itely:279: a4 a, c' f,
No durs yet.

http://codereview.appspot.com/1056041/diff/1/4#newcode314
Documentation/learning/tutorial.itely:314: 
We should still state the default.

http://codereview.appspot.com/1056041/diff/1/4#newcode487
Documentation/learning/tutorial.itely:487: @w{@samp{@{ C4 D E @}}} will produce
an error message.
Please don't add a 4 here; don't confuse the issue of case-sensitive.

http://codereview.appspot.com/1056041/diff/1/4#newcode506
Documentation/learning/tutorial.itely:506: c4 d e
This 4 is ok.

http://codereview.appspot.com/1056041/diff/1/4#newcode680
Documentation/learning/tutorial.itely:680: c4-\markup {
This isn't a verbatim example, so why change it?

http://codereview.appspot.com/1056041/diff/1/5
File Documentation/learning/tweaks.itely (right):

http://codereview.appspot.com/1056041/diff/1/5#newcode233
Documentation/learning/tweaks.itely:233: c4 d
Half notes might work better here.

http://codereview.appspot.com/1056041/diff/1/5#newcode3551
Documentation/learning/tweaks.itely:3551: global = {
PDP +104%.  Was it too complicated before?  Given its place in tweaks, I think
it was fine.

http://codereview.appspot.com/1056041/diff/1/5#newcode3650
Documentation/learning/tweaks.itely:3650: mpdolce =
By contrast, I (mostly) like this expansion, since it makes the material being
explained (scheme) easier to understand.

I'm not totally convinced about having #(make-dynamic-script on a new line, but
I'm find with this change with the understanding that it might change after
GLISS.

[Mark Polesky, 2010-05-02 20:13:04]

I wasn't sure if I should put all those points in the commit
message or not.  I'm curious to know what others think about
that.  For a general response to the first set of comments,
see http://lists.gnu.org/archive/html/lilypond-devel/2010-05/msg00032.html

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode148
Documentation/learning/common-notation.itely:148: LilyPond makes a sharp
distinction between musical content and
On 2010/05/02 16:01:33, Graham Percival wrote:
> change this "sharp" to "clear" ?

Okay, but in a separate patch.

http://codereview.appspot.com/1056041/diff/1/2#newcode155
Documentation/learning/common-notation.itely:155: rules.  The pitches in your
music are works of art, so they will
On 2010/05/02 16:01:33, Graham Percival wrote:
> I don't follow the "works of art" comment. 
> Could this be rewritten?

Yeah, I don't like it either.  But this patch should stick
to formatting issues alone.

http://codereview.appspot.com/1056041/diff/1/2#newcode484
Documentation/learning/common-notation.itely:484: f8
On 2010/05/02 16:01:33, Graham Percival wrote:
> I'm not certain that we want to split up
> \partial 8 f8

To me, it's easier to find the first note of a measure if
it's really the first thing on a line.  I'll leave it open
for debate.

http://codereview.appspot.com/1056041/diff/1/3
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/1/3#newcode447
Documentation/learning/fundamental.itely:447: } {
On 2010/05/02 16:01:33, Graham Percival wrote:
> I don't think that { f8 f c } is hard to understand.

That was a mistake -- I only meant to move the `}' off of
line 446.  Thanks.

http://codereview.appspot.com/1056041/diff/1/3#newcode726
Documentation/learning/fundamental.itely:726: { c8 b16 a b8 g~ g2 }
On 2010/05/02 16:01:33, Graham Percival wrote:
> I personally prefer having [...] g ~ g

I might insist on `g~ g'.  We'll see what the consensus
says.

http://codereview.appspot.com/1056041/diff/1/3#newcode1369
Documentation/learning/fundamental.itely:1369: }
On 2010/05/02 16:01:33, Graham Percival wrote:
> If we have barline checks for the first few bars,
> please leave the final barline check on the line.

Well, as it stands, they're not complete measures.  I
suppose you'd prefer adding r4 to each of the music
blocks above?

http://codereview.appspot.com/1056041/diff/1/3#newcode2098
Documentation/learning/fundamental.itely:2098: c4 d
On 2010/05/02 16:01:33, Graham Percival wrote:
> Why not make these
> c2 d
> to keep the one-bar-per-line idea?

The idea was not to *exceed* one bar per line.  It's not
always possible to have a complete bar occupy just one line,
so I didn't bother changing things like that here.

http://codereview.appspot.com/1056041/diff/1/3#newcode2537
Documentation/learning/fundamental.itely:2537: <<
On 2010/05/02 16:01:33, Graham Percival wrote:
> Indentation mistake!

Yes, that's intentional.  Read LM 3.4.1 "Soprano and cello"
in context.

http://codereview.appspot.com/1056041/diff/1/3#newcode3097
Documentation/learning/fundamental.itely:3097: 
On 2010/05/02 16:01:33, Graham Percival wrote:
> My eyes would prefer \keyTime.

Mine too.  I'll change it.

http://codereview.appspot.com/1056041/diff/1/3#newcode3153
Documentation/learning/fundamental.itely:3153: 
On 2010/05/02 16:01:33, Graham Percival wrote:
> Was the original example difficult to understand without
> having spaces between every four-line "stanza" ?

Well, as a general guideline, there might be places where
adding empty lines make the file easier to navigate.  For
example, in a situation like this, I can use Vim's `}'
function to rapidly (and repeatedly) jump to the next empty
line without needing to use the bracket-matching function
(which is slower because I have to move to the opening curly
brace for each jump).  Especially if the blocks are long,
this greatly facilitates file navigation.

http://codereview.appspot.com/1056041/diff/1/3#newcode3245
Documentation/learning/fundamental.itely:3245: @c TODO Avoid padtext - not
needed with skylining
On 2010/05/02 16:01:33, Graham Percival wrote:
> Dude!  If you're going to all this trouble to format
> stuff, and see a TODO like this, couldn't you do
> something about it?

I'm honestly not sure what the best way to solve it would
be.  The long-ish \padText command actually helps make the
point in the following example about how hard things can be
to read when *not* using variables.  Although now I see a
reference to "the last line" which I would need to change to
"the last measure" (or something) if this stays.

http://codereview.appspot.com/1056041/diff/1/3#newcode3312
Documentation/learning/fundamental.itely:3312: #(define-music-function
On 2010/05/02 16:01:33, Graham Percival wrote:
> I like this spacing expansion.

http://lists.gnu.org/archive/html/lilypond-devel/2010-04/msg00379.html

http://codereview.appspot.com/1056041/diff/1/4
File Documentation/learning/tutorial.itely (right):

http://codereview.appspot.com/1056041/diff/1/4#newcode81
Documentation/learning/tutorial.itely:81: will produce an error message.
On 2010/05/02 16:01:33, Graham Percival wrote:
> The focus is the case sensitivity.

I just don't like seeing the statement:
  `{ c d e }' is valid input.

To me, `{ c d e }' is poorly formatted input.

http://codereview.appspot.com/1056041/diff/1/4#newcode212
Documentation/learning/tutorial.itely:212: c4 d e f
On 2010/05/02 16:01:33, Graham Percival wrote:
> Again, no durations yet; they haven't been introduced.

Percent-sign comments and octave quotes haven't been
introduced either.  But I guess I see your point.

http://codereview.appspot.com/1056041/diff/1/4#newcode314
Documentation/learning/tutorial.itely:314: 
On 2010/05/02 16:01:33, Graham Percival wrote:
> We should still state the default.

Oops!  I meant to change that back but I forgot.  Disregard
that for now.

http://codereview.appspot.com/1056041/diff/1/4#newcode680
Documentation/learning/tutorial.itely:680: c4-\markup {
On 2010/05/02 16:01:33, Graham Percival wrote:
> This isn't a verbatim example, so why change it?

On principle.

http://codereview.appspot.com/1056041/diff/1/5
File Documentation/learning/tweaks.itely (right):

http://codereview.appspot.com/1056041/diff/1/5#newcode3650
Documentation/learning/tweaks.itely:3650: mpdolce =
On 2010/05/02 16:01:33, Graham Percival wrote:
> I'm not totally convinced about having
> #(make-dynamic-script on a new line, but

The practice can prevent needlessly long lines:

unavoidablyLongName = #(make-dynamic-script
                        (markup [lots of
                                      [nested markups

[Carl, 2010-05-03 13:48:52]

I'm out of time to finish this review today, so I thought it would be best to
publish what I have.

My overall thought is that in the Learning Manual, we shouldn't enforce
yet-to-be-explained coding standards.  Instead, we ought to format the examples
to do the best job possible of explaining the particular item the example is
trying to explain.

Later in Learning, when we talk about larger projects, we should introduce the
code standards and even give a link to Contributors.  In fact, I might prefer to
have the coding standards mentioned in Learning and linked to from Contributors,
but I'm not positive about that.

Thanks,

Carl

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode101
Documentation/learning/common-notation.itely:101: aeses1
During the GDP, we established the policy that simple lines such as this could
be on one line, even though they had multiple bars.

Perhaps we could just change the durations in this to 4, and we wouldn't even
be having a discussion.

http://codereview.appspot.com/1056041/diff/1/2#newcode226
Documentation/learning/common-notation.itely:226: c4~ c8 a~ a2
I don't think a requirement to eliminate unnecessary durations is wise.

Extra durations cause no problem at all, and may even make the music more
readable.

Making sure we have a minimum set of specified durations (start of measure or
start of line) makes sense to me.

http://codereview.appspot.com/1056041/diff/1/2#newcode253
Documentation/learning/common-notation.itely:253: b'2 a4 cis,\)
Add bar check, IMO.

In terms of the core concept for this snippet, the slur and the phrasing slur, I
think the original snippet shows the structure more clearly, with the () nested
inside the \( \).  So on a strictly pedagogical basis, I would tend to violate
the general code formatting rules for this case and leave it as-is.

http://codereview.appspot.com/1056041/diff/1/2#newcode271
Documentation/learning/common-notation.itely:271: fis2 g)
Same comment as for the previous snippet.

http://codereview.appspot.com/1056041/diff/1/2#newcode299
Documentation/learning/common-notation.itely:299: c4-+ c-_
Since the objective here is to show the articulations, we may not want to worry
about splitting the line.

If we do want to split the line, we should find two more articulations and add
them, so we get two complete bars, and put a bar check in the middle.

http://codereview.appspot.com/1056041/diff/1/2#newcode365
Documentation/learning/common-notation.itely:365: c2 c\!
See my comment at line 289

http://codereview.appspot.com/1056041/diff/1/2#newcode390
Documentation/learning/common-notation.itely:390: a1_"legato"
Repeat of 289

http://codereview.appspot.com/1056041/diff/1/2#newcode982
Documentation/learning/common-notation.itely:982: d4 b8 g4.
Bar checks here.

Pedagogically, it may be nice to have the notes and the lyrics have lines of the
same length.

[Graham Percival (old account), 2010-05-03 14:19:08]

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode101
Documentation/learning/common-notation.itely:101: aeses1
On 2010/05/03 13:48:52, Carl wrote:
> During the GDP, we established the policy that simple lines such as this could
> be on one line, even though they had multiple bars.
> 
> Perhaps we could just change the durations in this to 4, and we wouldn't even
> be having a discussion.

Already thought of that, but the 1 durations are so that we avoid having
cancellation naturals.  And we definitely don't want to add \override Voice
#'accidental-cancelletional = ##f  (or whatever the command is) here!

[Mark Polesky, 2010-05-04 04:41:46]

As I just mentioned in a lilypond-devel post, the problem
with using bar checks in the LM is that they're not
explained anywhere there (they're covered in the NR).  Maybe
a simple solution would be to mention them briefly near the
beginning of the LM, and then use them liberally throughout.

- Mark

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode101
Documentation/learning/common-notation.itely:101: aeses1
On 2010/05/03 14:19:08, Graham Percival wrote:
> the 1 durations are so that we avoid having cancellation > naturals.

That won't happen with these 4 pitches, since none of them
use the same letter name.  I'm changing them to quarters.

http://codereview.appspot.com/1056041/diff/1/2#newcode253
Documentation/learning/common-notation.itely:253: b'2 a4 cis,\)
On 2010/05/03 13:48:52, Carl wrote:
> I think the original snippet shows the structure more
> clearly, with the () nested inside the \( \).

Bar checks have not yet been introduced in the text.
I'd like to rewrite it using a single measure.  Would
you be okay with this?
  g4\( g8( a) b( c) b4\)

http://codereview.appspot.com/1056041/diff/1/2#newcode271
Documentation/learning/common-notation.itely:271: fis2 g)
On 2010/05/03 13:48:52, Carl wrote:
> Same comment as for the previous snippet.

Okay to change it to this single measure example?
  c4~( c8 d~ d4 e)

http://codereview.appspot.com/1056041/diff/1/2#newcode365
Documentation/learning/common-notation.itely:365: c2 c\!
On 2010/05/03 13:48:52, Carl wrote:
> See my comment at line 289

Can I make them all quarter notes?

http://codereview.appspot.com/1056041/diff/1/2#newcode390
Documentation/learning/common-notation.itely:390: a1_"legato"
On 2010/05/03 13:48:52, Carl wrote:
> Repeat of 289

How about half notes?

http://codereview.appspot.com/1056041/diff/1/2#newcode982
Documentation/learning/common-notation.itely:982: d4 b8 g4.
On 2010/05/03 13:48:52, Carl wrote:
> Bar checks here.
>
> Pedagogically, it may be nice to have the notes and the
> lyrics have lines of the same length.

Bar checks have still not been explained at this point.

WRT the lyrics line lengths, I thought about it, but the
*poetic* meter gets mangled by the *musical* meter.  Maybe
this makes it easier to see the note/word correspondence,
but it just looks weird:

  Girls and boys come
  out to play, The
  moon doth shine as
  bright as day;

  A --
  way in a __
  man -- ger, no __
  crib for a
  bed, __

  Let
  flee -- cy flocks the
  hills a --
  dorn,

[Carl, 2010-05-04 05:11:47]

http://codereview.appspot.com/1056041/diff/1/2
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/1/2#newcode101
Documentation/learning/common-notation.itely:101: aeses1
On 2010/05/03 14:19:08, Graham Percival wrote:
> On 2010/05/03 13:48:52, Carl wrote:

> > Perhaps we could just change the durations in this to 4, and we wouldn't
even
> > be having a discussion.
> 
> Already thought of that, but the 1 durations are so that we avoid having
> cancellation naturals.  And we definitely don't want to add \override Voice
> #'accidental-cancelletional = ##f  (or whatever the command is) here!


We don't have a key signature, and the notes aren't repeated , so there aren't
any cancellations in this snippet.  I just ran it to make sure.

In Notation, we had a cancellation problem with chords, which caused us to use
whole notes.  But I don't think that applies here.

THanks,

Carl

http://codereview.appspot.com/1056041/diff/1/2#newcode253
Documentation/learning/common-notation.itely:253: b'2 a4 cis,\)
On 2010/05/04 04:41:46, Mark Polesky wrote:
> Bar checks have not yet been introduced in the text.
> I'd like to rewrite it using a single measure.  Would
> you be okay with this?
>   g4\( g8( a) b( c) b4\)

Absolutely, as long as it looks reasonable.

But I think we should focus on *good examples* here, not *good source code
formatting*.  So since bar checks haven't been introduced, I withdraw all my
suggestions about bar checks.

http://codereview.appspot.com/1056041/diff/1/2#newcode271
Documentation/learning/common-notation.itely:271: fis2 g)
On 2010/05/04 04:41:46, Mark Polesky wrote:
> On 2010/05/03 13:48:52, Carl wrote:
> > Same comment as for the previous snippet.
> 
> Okay to change it to this single measure example?
>   c4~( c8 d~ d4 e)

I haven't compiled it, but I think a short example is better than a long
example.

http://codereview.appspot.com/1056041/diff/1/2#newcode365
Documentation/learning/common-notation.itely:365: c2 c\!
On 2010/05/04 04:41:46, Mark Polesky wrote:
> On 2010/05/03 13:48:52, Carl wrote:
> > See my comment at line 289
> 
> Can I make them all quarter notes?

Absolutely, as far as I'm concerned.  Unless the spacing gets too close...

http://codereview.appspot.com/1056041/diff/1/2#newcode982
Documentation/learning/common-notation.itely:982: d4 b8 g4.
On 2010/05/04 04:41:46, Mark Polesky wrote:
> On 2010/05/03 13:48:52, Carl wrote:
> > Bar checks here.
> >
> > Pedagogically, it may be nice to have the notes and the
> > lyrics have lines of the same length.
> 
> Bar checks have still not been explained at this point.
> 
> WRT the lyrics line lengths, I thought about it, but the
> *poetic* meter gets mangled by the *musical* meter.  Maybe
> this makes it easier to see the note/word correspondence,
> but it just looks weird:

No, you misunderstood my request.  I don't want to change the lyrics, I want to
change the notes.  Put one lyric line's worth of notes on a line.  And go ahead
and omit the bar check.

The pedagogy here should be focused on getting lyrics to align with notes, not
on proper formatting for LilyPond files.  Let that pedagogy trump for each of
these Learning examples; add a section for style that explains the recommended
style for complex music.

[Carl, 2010-05-05 10:57:06]

Hi Mark,

I think it's an improvement.  I've made specific comments inline.

Thanks!

Carl

http://codereview.appspot.com/1056041/diff/11001/12001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12001#newcode313
Documentation/learning/common-notation.itely:313: c4-^ c-+ c-- c-| | c4-> c-.
c2-_ |
Why 2 bar checks?

http://codereview.appspot.com/1056041/diff/11001/12001#newcode927
Documentation/learning/common-notation.itely:927: Girls and boys come | out to
play,
Do you like putting bar checks in Lyrics?  I've never done that.  Maybe I
should.

http://codereview.appspot.com/1056041/diff/11001/12001#newcode1339
Documentation/learning/common-notation.itely:1339: c,4 d, e, f, | g,4 a, b, c |
d4 e f g | a4 b c' d' |
Why move this from multiple lines to a single line?  This example seems to me to
be one where the pedagogy isn't tied to a single line, so perhaps it's a good
one to do the "one measure per line" rule.

http://codereview.appspot.com/1056041/diff/11001/12002
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12002#newcode1516
Documentation/learning/fundamental.itely:1516: dum dum | dum dum |
I think you have made an interesting choice here to reorder the content.  This
is an argument I have with myself, and I come up with different answers on
different projects.  Should I group everything by musical section (as you've
done here), or by content type (as was done previously).  I *think* my personal
preference in by content type, but I suppose that could change next week...

http://codereview.appspot.com/1056041/diff/11001/12002#newcode1585
Documentation/learning/fundamental.itely:1585: cis4 cis2.
Why not a bar check here?

http://codereview.appspot.com/1056041/diff/11001/12002#newcode1586
Documentation/learning/fundamental.itely:1586: g4
Why an incomplete bar?

http://codereview.appspot.com/1056041/diff/11001/12002#newcode2237
Documentation/learning/fundamental.itely:2237: \new Voice \relative c' {
I think the GDP code standards say that it should be \new Voice {
\relative c' {

http://codereview.appspot.com/1056041/diff/11001/12002#newcode2258
Documentation/learning/fundamental.itely:2258: \new Voice \relative c' {
I think the proper fix for this is to add the { after \new Voice

http://codereview.appspot.com/1056041/diff/11001/12002#newcode2280
Documentation/learning/fundamental.itely:2280: \new Staff <<
Aha -- I finally have figured out why I see so may examples of 
\new Staff << >>.  This has caused problems later on.

I recommend that we change all of these \new Staff << >> to \new Staff { }

http://codereview.appspot.com/1056041/diff/11001/12002#newcode3185
Documentation/learning/fundamental.itely:3185: @c Skylining handles this
correctly without padText
How about changing the padText command to a textFont command that does an
override on font size?

http://codereview.appspot.com/1056041/diff/11001/12003
File Documentation/learning/tutorial.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12003#newcode501
Documentation/learning/tutorial.itely:501: thumb is to indent code blocks with
either a tab or two spaces:
If our standard is two spaces, we should say two spaces, instead of a tab or two
spaces.

http://codereview.appspot.com/1056041/diff/11001/12003#newcode679
Documentation/learning/tutorial.itely:679: c4-\markup {
I don't prefer a standard that says we need to break up this markup.  The markup
fits comfortably on one line, And I think it's better keeping it as an
integrated whole.  If it didn't fit on one line, then I could see using this
formatting.

[Graham Percival (old account), 2010-05-05 17:57:41]

http://codereview.appspot.com/1056041/diff/11001/12001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12001#newcode313
Documentation/learning/common-notation.itely:313: c4-^ c-+ c-- c-| | c4-> c-.
c2-_ |
On 2010/05/05 10:57:06, Carl wrote:
> Why 2 bar checks?

The first one is an articulation.  Hard to read, I agree.

In this case, for pedagogical reasons, I'd split it over two lines and remove
the barline checks.

[Graham Percival, 2010-05-05 18:01:31]

On Wed, May 5, 2010 at 11:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
> I think it's an improvement.  I've made specific comments inline.

sweet mao... you're adding barline checks to every single example?!

  cis4 ees fisis, aeses |

I definitely do **not** like this.
  cis4 ees fisis, aeses
is just fine to read.  If there's only one bar of simple music on a
line, surely we don't need a barline check!

Cheers,
- Graham

[Graham Percival (old account), 2010-05-05 18:14:18]

I've only reviewed the first file.  I don't think that reviewing more would do
good; we're still debating simple things like "should there be a barline check
at the end of every single line".  Seeing a patch doesn't help with that debate.

At the same time, there's good changes -- like adding explicit durations -- that
are getting delayed / possibly lost in the shuffle.

I don't know what the answer is... I mean, if I had a patch that only did the
everybody-agreed-upon things (like the explicit durations), I'd happily push
that, and then we could continue debating the contentious stuff.  OTOH, if I
were you, I wouldn't want to put this patch, create a new patch with only the
durations (and whatever else we agree on), push that, then rebase my original
patch to match the new git thing.  OTOH*2, maybe you're getting sick of having
long discussions about patches, and would like to get *something* pushed.

http://codereview.appspot.com/1056041/diff/11001/12001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12001#newcode409
Documentation/learning/common-notation.itely:409: a2_\markup {
What was wrong with the 1 durations?  Why change to 2 ?

http://codereview.appspot.com/1056041/diff/11001/12001#newcode492
Documentation/learning/common-notation.itely:492: \partial 8 f8 | c2 d |
I'd rather see a newline inside here.  We could debate about whether the f8
should be on the same line as the \partial 8 or on a separate line, but the "c2
d" definitely belongs on a new line.

http://codereview.appspot.com/1056041/diff/11001/12001#newcode515
Documentation/learning/common-notation.itely:515: \times 2/3 { d4 a8 } |
The barline check here does nothing to help us understand the example.

http://codereview.appspot.com/1056041/diff/11001/12001#newcode974
Documentation/learning/common-notation.itely:974: @c TODO: If we keep the bar
checks in the lyrics above, we should
As you know, I'm vehemently opposed to adding TODOs to "finished" docs.  I'm ok
with this right now since we're just discussing the patch, but it had better not
be pushed with this still in here.

http://codereview.appspot.com/1056041/diff/11001/12001#newcode1339
Documentation/learning/common-notation.itely:1339: c,4 d, e, f, | g,4 a, b, c |
d4 e f g | a4 b c' d' |
On 2010/05/05 10:57:06, Carl wrote:
> Why move this from multiple lines to a single line?  This example seems to me
to
> be one where the pedagogy isn't tied to a single line, so perhaps it's a good
> one to do the "one measure per line" rule.

Agreed.  This example was just fine to begin with, other than adding a 4 to the
beginning of each line.

[c_sorensen, 2010-05-06 00:04:47]

On 5/5/10 12:01 PM, "Graham Percival" <graham@percival-music.ca> wrote:

> On Wed, May 5, 2010 at 11:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
>> I think it's an improvement.  I've made specific comments inline.
> 
> sweet mao... you're adding barline checks to every single example?!
> 
>   cis4 ees fisis, aeses |
> 
> I definitely do **not** like this.
>   cis4 ees fisis, aeses
> is just fine to read.  If there's only one bar of simple music on a
> line, surely we don't need a barline check!

For the Learning Manual, I would dispense with *all* final barline checks.

Carl

[Graham Percival, 2010-05-06 00:11:26]

On Wed, May 05, 2010 at 06:04:42PM -0600, Carl Sorensen wrote:
> 
> On 5/5/10 12:01 PM, "Graham Percival" <graham@percival-music.ca> wrote:
> 
> > On Wed, May 5, 2010 at 11:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
> >> I think it's an improvement.  I've made specific comments inline.
> > 
> > sweet mao... you're adding barline checks to every single example?!
> > 
> >   cis4 ees fisis, aeses |
> 
> For the Learning Manual, I would dispense with *all* final barline checks.

I wouldn't go that far.  Compare:
  r8 | r4. r4 c8 | a'([ g)] f f([ e)] d | e([ d)] c bes' |
with
  r8 | r4. r4 c8 | a'([ g)] f f([ e)] d | e([ d)] c bes'

I'd rather have the first type; the final barline check just
reassures you that everything matches up.

Cheers,
- Graham

[c_sorensen, 2010-05-06 00:31:38]

On 5/5/10 6:11 PM, "Graham Percival" <graham@percival-music.ca> wrote:

> On Wed, May 05, 2010 at 06:04:42PM -0600, Carl Sorensen wrote:
>> 
>> On 5/5/10 12:01 PM, "Graham Percival" <graham@percival-music.ca> wrote:
>> 
>>> On Wed, May 5, 2010 at 11:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
>>>> I think it's an improvement.  I've made specific comments inline.
>>> 
>>> sweet mao... you're adding barline checks to every single example?!
>>> 
>>>   cis4 ees fisis, aeses |
>> 
>> For the Learning Manual, I would dispense with *all* final barline checks.
> 
> I wouldn't go that far.  Compare:
>   r8 | r4. r4 c8 | a'([ g)] f f([ e)] d | e([ d)] c bes' |
> with
>   r8 | r4. r4 c8 | a'([ g)] f f([ e)] d | e([ d)] c bes'
> 
> I'd rather have the first type; the final barline check just
> reassures you that everything matches up.

OK.  Final barline checks on examples where they add to the clarity of *that
particular* examples would be fine with me.  But when I reviewed the patch,
I felt that the final bar checks actually got in the way for me on virtually
every example.

Thanks,

Carl

[Graham Percival, 2010-05-06 00:34:41]

On Wed, May 05, 2010 at 06:31:21PM -0600, Carl Sorensen wrote:
> On 5/5/10 6:11 PM, "Graham Percival" <graham@percival-music.ca> wrote:
> 
> >>> sweet mao... you're adding barline checks to every single example?!

> OK.  Final barline checks on examples where they add to the clarity of *that
> particular* examples would be fine with me.

Absolutely.

> But when I reviewed the patch,
> I felt that the final bar checks actually got in the way for me on virtually
> every example.

Absolutely.  My highly-quoted text above should be read in a tone
of horror.

Cheers,
- Graham

[markpolesky_yahoo.com, 2010-05-06 07:24:04]

Patch set 3 uploaded.
http://codereview.appspot.com/1056041

Hope this isn't driving you crazy.

- Mark


      

[Trevor Daniels, 2010-05-06 08:48:45]

Mark
I've not examined every change in detail, but I think I've picked out all the
ones I wanted to comment on.
Trevor

http://codereview.appspot.com/1056041/diff/24001/25001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25001#newcode1359
Documentation/learning/common-notation.itely:1359: }
I agree with Carl and Graham - it looked better as it was originally, on single
lines.  It is a scale, not barred music, although I've no problem with the bar
checks.

http://codereview.appspot.com/1056041/diff/24001/25002
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1255
Documentation/learning/fundamental.itely:1255: }
I think this is one case where the final bar check should be added.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1349
Documentation/learning/fundamental.itely:1349: We end with an example to show
how we might code a solo verse which
I put this in after several questions on -user about how this should be done,
but I wasn't very happy with it.  If you can come up with a better way of coding
a solo verse going into a two-part refain let's change it.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1702
Documentation/learning/fundamental.itely:1702: \key g \minor
Since we placed \clef "treble" in the LM I don't remember any questions on -user
about G_8 failing.  We're adding many superfluous braces, quotes round variable
names, etc, why leave these out?  But if the quotes are to be removed we need to
add a remark somewhere about them being required sometimes.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3163
Documentation/learning/fundamental.itely:3163: }
There's no point in fiddling with this example - it needs replacing.

http://codereview.appspot.com/1056041/diff/24001/25004
File Documentation/learning/tweaks.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25004#newcode1651
Documentation/learning/tweaks.itely:1651: c2^"Text3" c^"Text4" |
I think in this case the original layout illustrated the point more clearly

http://codereview.appspot.com/1056041/diff/24001/25004#newcode2131
Documentation/learning/tweaks.itely:2131: c2^"Text3" c^"Text4" |
Ditto

http://codereview.appspot.com/1056041/diff/24001/25004#newcode2159
Documentation/learning/tweaks.itely:2159: c2^"Text3" c^"Text4" |
Ditto

[Graham Percival (old account), 2010-05-06 13:53:26]

Looking much closer now.

http://codereview.appspot.com/1056041/diff/24001/25001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25001#newcode147
Documentation/learning/common-notation.itely:147: a1 |
This example doesn't need a bar check.

http://codereview.appspot.com/1056041/diff/24001/25001#newcode541
Documentation/learning/common-notation.itely:541: c2 \grace { a32[ b] } c2 |
I don't think we need bar checks here, either.

http://codereview.appspot.com/1056041/diff/24001/25001#newcode1088
Documentation/learning/common-notation.itely:1088: d4 | g4 g a8( b) | g4 g b8(
c) |
Since the first \partial example puts the note on the same line as the \partial,
let's do the same thing here:
  \partial 4 d4 |

http://codereview.appspot.com/1056041/diff/24001/25001#newcode1359
Documentation/learning/common-notation.itely:1359: }
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> I agree with Carl and Graham - it looked better as it was originally, on
single
> lines.  It is a scale, not barred music, although I've no problem with the bar
> checks. 

I agree with Trevor agreeing with me.  :)

http://codereview.appspot.com/1056041/diff/24001/25001#newcode1370
Documentation/learning/common-notation.itely:1370: b'8. cis''16 b'8 d''4 d''8 |
For the record, these bar checks are very good.

http://codereview.appspot.com/1056041/diff/24001/25002
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25002#newcode239
Documentation/learning/fundamental.itely:239: @code{keyTime},
@code{pianorighthand}, or @code{foofoobarbaz}.
Could we change the foofoobarbaz to something else?  In general, I'd rather
avoid programming jokes in the docs.

(I know that I've added "foo" a few times, but I try not to... this is something
on my todo list for GDP2, but since you're changing this spot anyway...)

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1255
Documentation/learning/fundamental.itely:1255: }
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> I think this is one case where the final bar check should be added.  

+1

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1304
Documentation/learning/fundamental.itely:1304: AltoMusic  = \relative c' { c4 |
c4. c8 e4  e    | f4   f   e  }
Same here -- please keep the final barline check.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1702
Documentation/learning/fundamental.itely:1702: \key g \minor
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> Since we placed \clef "treble" in the LM I don't remember any questions on
-user
> about G_8 failing.  We're adding many superfluous braces, quotes round
variable
> names, etc, why leave these out?  But if the quotes are to be removed we need
to
> add a remark somewhere about them being required sometimes.

We already have that remark in Notation.  I'm not too fussed whether we have
quotes or not, but it _does_ seem weird to remove the quotes if they're already
there.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode2481
Documentation/learning/fundamental.itely:2481: <<
Indentation mistake.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode2915
Documentation/learning/fundamental.itely:2915: \key c \minor
I think this one can be condensed onto one line as well.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3053
Documentation/learning/fundamental.itely:3053: \key c \minor
Condense.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3133
Documentation/learning/fundamental.itely:3133: \fragmentA \fragmentA |
Oooh, I really like the barline check there.  It hadn't occurred to me you could
do that.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3163
Documentation/learning/fundamental.itely:3163: }
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> There's no point in fiddling with this example - it needs replacing.

+1
that said, there's also no point debating the formatting.  :)

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3249
Documentation/learning/fundamental.itely:3249: cis4 f |
I don't see the point of exapanding these examples, but I won't insist that you
change them back.

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3352
Documentation/learning/fundamental.itely:3352: r4 f8 a |
Actually, I take that back -- this example looks more complicated than it needs
to be.  I think it looked better in the original version, and since it's
comprised of sticking together the previous examples, those should also be
retained in "condensed" form.

http://codereview.appspot.com/1056041/diff/24001/25004
File Documentation/learning/tweaks.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25004#newcode388
Documentation/learning/tweaks.itely:388: a4^Black
sweet mao, this actually works?!  ick.

Please add "" around the "Black".  Ditto for Red and Green.

http://codereview.appspot.com/1056041/diff/24001/25004#newcode1651
Documentation/learning/tweaks.itely:1651: c2^"Text3" c^"Text4" |
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> I think in this case the original layout illustrated the point more clearly

+1

[Mark Polesky, 2010-05-06 17:27:09]

http://codereview.appspot.com/1056041/diff/11001/12001
File Documentation/learning/common-notation.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12001#newcode409
Documentation/learning/common-notation.itely:409: a2_\markup {
On 2010/05/05 18:14:19, Graham Percival wrote:
> What was wrong with the 1 durations?  Why change to 2 ?

Well, though this wasn't my initial reason, now I see
that with half notes, each example will be a single
measure; then I can get rid of the bar checks
altogether.

http://codereview.appspot.com/1056041/diff/11001/12001#newcode927
Documentation/learning/common-notation.itely:927: Girls and boys come | out to
play,
On 2010/05/05 10:57:06, Carl wrote:
> Do you like putting bar checks in Lyrics?  I've never
> done that.  Maybe I should.

Doing so can catch subtle errors that you might
otherwise miss.

http://codereview.appspot.com/1056041/diff/11001/12002
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/11001/12002#newcode2237
Documentation/learning/fundamental.itely:2237: \new Voice \relative c' {
On 2010/05/05 10:57:06, Carl wrote:
> I think the GDP code standards say that it should be \new Voice {
> \relative c' {

Oh.  Where are the GDP code standards?  Should I also
put a { after the \with block that ends on line 2232?

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1255
Documentation/learning/fundamental.itely:1255: \score {
On 2010/05/06 08:48:46, Trevor Daniels wrote:
> I think this is one case where the final bar check should
> be added.

Then we get barcheck warnings.  Those are incomplete
measures, and I'd rather keep them that way, since that's
the musical phrase.  In the original oratorio, the last
eighth note of that measure starts the next phrase.  Do we
really need to add an eighth rest where there isn't one?  Is
it such a bad thing to end with an incomplete measure in the
docs?  Vocal examples tend to start and end with incomplete
measures.  If I were typesetting this myself, I might
naturally stop to compile at the end of a phrase, even if
ends with an incomplete measure.

The original (in a different key, mind you) can be found on
p.187 of this pdf:
http://imslp.org/wiki/Special:ImagefromIndex/39856

http://codereview.appspot.com/1056041/diff/24001/25002#newcode2481
Documentation/learning/fundamental.itely:2481: \midi @{ @}
On 2010/05/06 13:53:26, Graham Percival wrote:
> Indentation mistake.

Yes, that's intentional.  Read LM 3.4.1 "Soprano and cello"
in context.

[Mark Polesky, 2010-05-07 08:05:09]

Okay, this better be good enough to push; it's starting
to get tiresome.  Thanks for all your input so far.  I'll
replace the original (protracted) commit message with
something far more conservative.

Graham, are you going to make me remove the TODO on line
1339 in fundamental.itely?  Also, Graham, I didn't make
every last change you proposed.  Things can always be
refined later.  As long as you guys think that this most
recent patch represents an improvement on the current
docs, I'd like to push it.

- Mark

http://codereview.appspot.com/1056041/diff/24001/25002
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/24001/25002#newcode1702
Documentation/learning/fundamental.itely:1702: \key g \minor
On 2010/05/06 13:53:26, Graham Percival wrote:
> I'm not too fussed whether we have quotes or not, but it
> _does_ seem weird to remove the quotes if they're already
> there.

What bothered me the most was the inconsistency...

http://codereview.appspot.com/1056041/diff/24001/25002#newcode3352
Documentation/learning/fundamental.itely:3352: r4 f8 a |
On 2010/05/06 13:53:26, Graham Percival wrote:
> Actually, I take that back -- this example looks more
> complicated than it needs to be.

Weird, I find things are much clearer with one bar per line.

[markpolesky_yahoo.com, 2010-05-07 08:10:23]

patch set 4 is up: http://codereview.appspot.com/1056041


      

[markpolesky_yahoo.com, 2010-05-07 08:10:23]

patch set 4 is up: http://codereview.appspot.com/1056041


      

[Graham Percival (old account), 2010-05-07 10:47:14]

Looks (mostly) good to me, other than the below 2 items.

http://codereview.appspot.com/1056041/diff/33001/27003
File Documentation/learning/fundamental.itely (right):

http://codereview.appspot.com/1056041/diff/33001/27003#newcode1339
Documentation/learning/fundamental.itely:1339: @c TODO: This extended example
(from here to the end of this node)
I don't want any commits to add TODOs to "finished" code; please move this to
your own personal todo list, or add an issue on the tracker for it.

http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481
Documentation/learning/fundamental.itely:2481: <<
There is *still* an indentation mistake here.  This is the third time I've
pointed it out -- do we disagree on the "two-space indents" rule?

[markpolesky_yahoo.com, 2010-05-07 20:31:36]

Graham Percival wrote:
> http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481
> Documentation/learning/fundamental.itely:2481: <<
> There is *still* an indentation mistake here.  This is the
> third time I've pointed it out -- do we disagree on the
> "two-space indents" rule?

Graham,

have you not been getting my comment replies?

I've explained this twice now:
http://codereview.appspot.com/1056041/diff/1/3#newcode2537
http://codereview.appspot.com/1056041/diff/24001/25002#newcode2481

Now I will explain it a third time.

The indentation is intentionally wrong.  It represents an
intermediate stage of the input code as new elements are
added, and before they are formatted.  The text is clear
about this and walks the user through the step of fixing the
indentation.

To see this yourself, go to LM 3.4.1 "Soprano and cello":
http://kainhofer.com/~lilypond/Documentation/learning/soprano-and-cello.html

Scroll down past the third example to the paragraph that
starts with "This is looking promising".  Read the next
couple of paragraphs in context:

* * * * * * * * * *

This is looking promising, but the cello part won’t appear
in the score – we haven’t used it in the \score section. If
we want the cello part to appear under the soprano part, we
need to add

  \new Staff \celloMusic

underneath the soprano stuff. We also need to add << and >>
around the music – that tells LilyPond that there’s more
than one thing (in this case, two Staves) happening at once.
The \score looks like this now:

  [ the example with bad indentation ]

This looks a bit messy; the indentation is messed up now.
That is easily fixed. Here’s the complete soprano and cello
template.

  [ the example with good indentation ]

* * * * * * * * * *

- Mark


      

[markpolesky_yahoo.com, 2010-05-07 20:31:36]

Graham Percival wrote:
> http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481
> Documentation/learning/fundamental.itely:2481: <<
> There is *still* an indentation mistake here.  This is the
> third time I've pointed it out -- do we disagree on the
> "two-space indents" rule?

Graham,

have you not been getting my comment replies?

I've explained this twice now:
http://codereview.appspot.com/1056041/diff/1/3#newcode2537
http://codereview.appspot.com/1056041/diff/24001/25002#newcode2481

Now I will explain it a third time.

The indentation is intentionally wrong.  It represents an
intermediate stage of the input code as new elements are
added, and before they are formatted.  The text is clear
about this and walks the user through the step of fixing the
indentation.

To see this yourself, go to LM 3.4.1 "Soprano and cello":
http://kainhofer.com/~lilypond/Documentation/learning/soprano-and-cello.html

Scroll down past the third example to the paragraph that
starts with "This is looking promising".  Read the next
couple of paragraphs in context:

* * * * * * * * * *

This is looking promising, but the cello part won’t appear
in the score – we haven’t used it in the \score section. If
we want the cello part to appear under the soprano part, we
need to add

  \new Staff \celloMusic

underneath the soprano stuff. We also need to add << and >>
around the music – that tells LilyPond that there’s more
than one thing (in this case, two Staves) happening at once.
The \score looks like this now:

  [ the example with bad indentation ]

This looks a bit messy; the indentation is messed up now.
That is easily fixed. Here’s the complete soprano and cello
template.

  [ the example with good indentation ]

* * * * * * * * * *

- Mark


      

[Graham Percival, 2010-05-07 20:46:54]

On Fri, May 07, 2010 at 01:31:35PM -0700, Mark Polesky wrote:
> Graham Percival wrote:
> > http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481
> > Documentation/learning/fundamental.itely:2481: <<
> > There is *still* an indentation mistake here.  This is the
> > third time I've pointed it out -- do we disagree on the
> > "two-space indents" rule?
> 
> Graham,
> 
> have you not been getting my comment replies?

Apparently not!

> I've explained this twice now:
> http://codereview.appspot.com/1056041/diff/1/3#newcode2537
> http://codereview.appspot.com/1056041/diff/24001/25002#newcode2481

Huh.  Yes, that makes perfect sense...


... oh, I see the problem now.  I can't (or at least, a year ago I
couldn't) use my google apps account for codereview, so I made a
throwaway gmail account for that, but never set up forwarding.  I
see your replies on that account.

Sorry about this.
- Graham