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

Issue 1056041: Doc: LM: Reformat ly code. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 11 months ago by Mark Polesky
Modified:
13 years, 10 months ago
Reviewers:
Graham Percival (old account), Trevor Daniels, carl.d.sorensen
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 74

Patch Set 2 : Make requested changes. #

Total comments: 21

Patch Set 3 : Requested changes. #

Total comments: 30

Patch Set 4 : Make requested changes. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+943 lines, -782 lines) Patch
M Documentation/learning/common-notation.itely View 1 2 3 43 chunks +112 lines, -95 lines 0 comments Download
M Documentation/learning/fundamental.itely View 1 2 3 66 chunks +383 lines, -331 lines 2 comments Download
M Documentation/learning/tutorial.itely View 1 2 6 chunks +14 lines, -12 lines 0 comments Download
M Documentation/learning/tweaks.itely View 1 2 3 98 chunks +434 lines, -344 lines 0 comments Download

Messages

Total messages: 26
Graham Percival (old account)
I have many concerns about the expanded input code. I also don't like seeing so ...
13 years, 11 months ago (2010-05-02 16:01:33 UTC) #1
Mark Polesky
I wasn't sure if I should put all those points in the commit message or ...
13 years, 11 months ago (2010-05-02 20:13:04 UTC) #2
Carl
I'm out of time to finish this review today, so I thought it would be ...
13 years, 11 months ago (2010-05-03 13:48:52 UTC) #3
Graham Percival (old account)
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 ...
13 years, 11 months ago (2010-05-03 14:19:08 UTC) #4
Mark Polesky
As I just mentioned in a lilypond-devel post, the problem with using bar checks in ...
13 years, 11 months ago (2010-05-04 04:41:46 UTC) #5
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 On 2010/05/03 14:19:08, Graham Percival wrote: > On ...
13 years, 11 months ago (2010-05-04 05:11:47 UTC) #6
Carl
13 years, 11 months ago (2010-05-04 05:11:50 UTC) #7
Carl
Hi Mark, I think it's an improvement. I've made specific comments inline. Thanks! Carl http://codereview.appspot.com/1056041/diff/11001/12001 ...
13 years, 11 months ago (2010-05-05 10:57:06 UTC) #8
Graham Percival (old account)
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-_ | ...
13 years, 11 months ago (2010-05-05 17:57:41 UTC) #9
Graham Percival
On Wed, May 5, 2010 at 11:57 AM, <Carl.D.Sorensen@gmail.com> wrote: > I think it's an ...
13 years, 11 months ago (2010-05-05 18:01:31 UTC) #10
Graham Percival (old account)
I've only reviewed the first file. I don't think that reviewing more would do good; ...
13 years, 11 months ago (2010-05-05 18:14:18 UTC) #11
c_sorensen
On 5/5/10 12:01 PM, "Graham Percival" <graham@percival-music.ca> wrote: > On Wed, May 5, 2010 at ...
13 years, 11 months ago (2010-05-06 00:04:47 UTC) #12
Graham Percival
On Wed, May 05, 2010 at 06:04:42PM -0600, Carl Sorensen wrote: > > On 5/5/10 ...
13 years, 11 months ago (2010-05-06 00:11:26 UTC) #13
c_sorensen
On 5/5/10 6:11 PM, "Graham Percival" <graham@percival-music.ca> wrote: > On Wed, May 05, 2010 at ...
13 years, 11 months ago (2010-05-06 00:31:38 UTC) #14
Graham Percival
On Wed, May 05, 2010 at 06:31:21PM -0600, Carl Sorensen wrote: > On 5/5/10 6:11 ...
13 years, 11 months ago (2010-05-06 00:34:41 UTC) #15
markpolesky_yahoo.com
Patch set 3 uploaded. http://codereview.appspot.com/1056041 Hope this isn't driving you crazy. - Mark
13 years, 11 months ago (2010-05-06 07:24:04 UTC) #16
Trevor Daniels
Mark I've not examined every change in detail, but I think I've picked out all ...
13 years, 11 months ago (2010-05-06 08:48:45 UTC) #17
Graham Percival (old account)
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 ...
13 years, 11 months ago (2010-05-06 13:53:26 UTC) #18
Mark Polesky
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: > ...
13 years, 11 months ago (2010-05-06 17:27:09 UTC) #19
Mark Polesky
Okay, this better be good enough to push; it's starting to get tiresome. Thanks for ...
13 years, 11 months ago (2010-05-07 08:05:09 UTC) #20
markpolesky_yahoo.com
patch set 4 is up: http://codereview.appspot.com/1056041
13 years, 11 months ago (2010-05-07 08:10:23 UTC) #21
markpolesky_yahoo.com
patch set 4 is up: http://codereview.appspot.com/1056041
13 years, 11 months ago (2010-05-07 08:10:23 UTC) #22
Graham Percival (old account)
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): ...
13 years, 11 months ago (2010-05-07 10:47:14 UTC) #23
markpolesky_yahoo.com
Graham Percival wrote: > http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481 > Documentation/learning/fundamental.itely:2481: << > There is *still* an indentation mistake ...
13 years, 10 months ago (2010-05-07 20:31:36 UTC) #24
markpolesky_yahoo.com
Graham Percival wrote: > http://codereview.appspot.com/1056041/diff/33001/27003#newcode2481 > Documentation/learning/fundamental.itely:2481: << > There is *still* an indentation mistake ...
13 years, 10 months ago (2010-05-07 20:31:36 UTC) #25
Graham Percival
13 years, 10 months ago (2010-05-07 20:46:54 UTC) #26
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
Sign in to reply to this message.

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