Re: Doc: LM: Reformat ly code. (issue1056041)

[Carl . D . Sorensen]

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.

http://codereview.appspot.com/1056041/show