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

Issue 1242044: Doc: NR: Reformat ly code. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by Mark Polesky
Modified:
5 years, 10 months ago
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Doc: NR: Reformat ly code.

Patch Set 1 #

Patch Set 2 : Second pass. #

Total comments: 30

Patch Set 3 : Make changes requested by Graham. #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+2130 lines, -2305 lines) Patch
M Documentation/notation/changing-defaults.itely View 1 2 58 chunks +230 lines, -250 lines 9 comments Download
M Documentation/notation/cheatsheet.itely View 3 chunks +9 lines, -9 lines 0 comments Download
M Documentation/notation/chords.itely View 1 2 38 chunks +94 lines, -105 lines 1 comment Download
M Documentation/notation/editorial.itely View 1 2 10 chunks +24 lines, -24 lines 0 comments Download
M Documentation/notation/expressive.itely View 1 2 28 chunks +104 lines, -99 lines 3 comments Download
M Documentation/notation/fretted-strings.itely View 1 2 45 chunks +213 lines, -316 lines 1 comment Download
M Documentation/notation/input.itely View 1 2 31 chunks +153 lines, -135 lines 0 comments Download
M Documentation/notation/keyboards.itely View 8 chunks +39 lines, -37 lines 0 comments Download
M Documentation/notation/percussion.itely View 1 18 chunks +49 lines, -51 lines 0 comments Download
M Documentation/notation/pitches.itely View 1 50 chunks +271 lines, -276 lines 0 comments Download
M Documentation/notation/repeats.itely View 1 12 chunks +40 lines, -41 lines 0 comments Download
M Documentation/notation/rhythms.itely View 1 2 57 chunks +178 lines, -178 lines 0 comments Download
M Documentation/notation/simultaneous.itely View 1 14 chunks +42 lines, -48 lines 0 comments Download
M Documentation/notation/spacing.itely View 1 2 52 chunks +207 lines, -237 lines 0 comments Download
M Documentation/notation/staff.itely View 1 28 chunks +85 lines, -98 lines 0 comments Download
M Documentation/notation/text.itely View 1 2 25 chunks +95 lines, -109 lines 0 comments Download
M Documentation/notation/unfretted-strings.itely View 3 chunks +6 lines, -6 lines 0 comments Download
M Documentation/notation/vocal.itely View 1 2 31 chunks +268 lines, -263 lines 0 comments Download
M Documentation/notation/wind.itely View 1 chunk +16 lines, -16 lines 0 comments Download
M Documentation/notation/world.itely View 3 chunks +7 lines, -7 lines 0 comments Download

Messages

Total messages: 5
Graham Percival (old account)
Well, there's 40 minutes of my 60 minutes of lilypond work for today done. What's ...
9 years, 1 month ago (2010-05-22 08:10:42 UTC) #1
Mark Polesky
Patch set 3 uploaded Would anyone else like to review this patch? Thanks - Mark ...
9 years, 1 month ago (2010-05-22 18:08:30 UTC) #2
markpolesky_yahoo.com
Patch set 3 uploaded. http://codereview.appspot.com/1242044 I replied to Graham's comments there, but a copy of ...
9 years, 1 month ago (2010-05-23 16:16:47 UTC) #3
Graham Percival (old account)
I only had time to examine changing-defaults. I'll review the other files tonight. http://codereview.appspot.com/1242044/diff/30002/34001 File ...
9 years, 1 month ago (2010-05-25 12:10:54 UTC) #4
Graham Percival (old account)
9 years, 1 month ago (2010-05-25 18:52:01 UTC) #5
I managed to trudge through another 4 or 5 files.  Look, this is ridiculous. 
I'm officially rejecting this patch because it's **WAY** too long for anybody to
look at.

Please break this into separate patches, where each patch is 1500 lines or less.
 If a single file has more than 1500 diff-lines, you can exceed the 1500-line
limit in order to send a patch for that file.


Start from pitches.itely, and go in order of appearance in the NR.  Don't submit
a new patch until the previous one is accepted and committed.

I'm sorry that this makes a lot more work for you, but I tried to warn you after
the LM-patch.  As we state on the "doc additions and suggestions" page, **ask**
before undertaking a huge doc-writing project.

http://codereview.appspot.com/1242044/diff/2001/3010
File Documentation/notation/pitches.itely (right):

http://codereview.appspot.com/1242044/diff/2001/3010#newcode2724
Documentation/notation/pitches.itely:2724: \aikenHeadsMinor
On 2010/05/22 18:08:30, Mark Polesky wrote:
> On 2010/05/22 08:10:42, Graham Percival wrote:
> > err, why?
> 
> why what?

... um.  I have to admit that I can't remember.  Sorry.

http://codereview.appspot.com/1242044/diff/2001/3015
File Documentation/notation/staff.itely (right):

http://codereview.appspot.com/1242044/diff/2001/3015#newcode1373
Documentation/notation/staff.itely:1373: { \voiceTwo d4. bes4 c8 }
On 2010/05/22 18:08:30, Mark Polesky wrote:
> The current version uses \StemDown and \StemUp for the main
> voice, and relies on the cue pitches for its stem
> directions, which all happen to point up in this particular
> case.  I think the \voiceOne (etc.) commands are preferable
> here.

Oops, I missed that.  Yeah, \voiceEtc is definitely better than \stemEtc.

http://codereview.appspot.com/1242044/diff/30002/34003
File Documentation/notation/chords.itely (right):

http://codereview.appspot.com/1242044/diff/30002/34003#newcode60
Documentation/notation/chords.itely:60: \chordmode { c1 | g1 | a1 | g1 | c1 | }
Ick, this looks much harder to understand.  As I said earlier, I'm not going to
insist that you remove this change, but I really hope that you don't continue
working on stuff like this.  It will be discussed during GLISS:
http://lilypond.org/~graham/gliss/specifics.html

http://codereview.appspot.com/1242044/diff/30002/34005
File Documentation/notation/expressive.itely (right):

http://codereview.appspot.com/1242044/diff/30002/34005#newcode151
Documentation/notation/expressive.itely:151: c4-> c-. c2-_ |
Could you switch the order of the c-| and c->   ?  Having the barline right
after c-|  doesn't look good.  Also, could you line the second barline under the
first?  That would help the eye distinguish the barline-punctuation from the
accent-punctuation.

http://codereview.appspot.com/1242044/diff/30002/34005#newcode265
Documentation/notation/expressive.itely:265: c2_\spp c^\ff |
Hmm.  It might be better if a barline check at the end of the line always had
two (or more) spaces before it, and if they lined up whenever possible.

Don't change anything now, but that's definitely an idea to discuss in GLISS. 
I've added it to the list.

http://codereview.appspot.com/1242044/diff/30002/34005#newcode320
Documentation/notation/expressive.itely:320: c2 b4 a | g1\espressivo |
I think this looked better on two lines.

http://codereview.appspot.com/1242044/diff/30002/34006
File Documentation/notation/fretted-strings.itely (right):

http://codereview.appspot.com/1242044/diff/30002/34006#newcode315
Documentation/notation/fretted-strings.itely:315: \relative c' {
the \relative should be on the same line as the "ties =".
Sign in to reply to this message.

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