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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 11 months ago by Mark Polesky
Modified:
3 years, 11 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 ...
3 years, 11 months ago #1
Mark Polesky
I wasn't sure if I should put all those points in the commit message or ...
3 years, 11 months ago #2
Carl
I'm out of time to finish this review today, so I thought it would be ...
3 years, 11 months ago #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 ...
3 years, 11 months ago #4
Mark Polesky
As I just mentioned in a lilypond-devel post, the problem with using bar checks in ...
3 years, 11 months ago #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 ...
3 years, 11 months ago #6
Carl
3 years, 11 months ago #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 ...
3 years, 11 months ago #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-_ | ...
3 years, 11 months ago #9
Graham Percival
On Wed, May 5, 2010 at 11:57 AM, <Carl.D.Sorensen@gmail.com> wrote: > I think it's an ...
3 years, 11 months ago #10
Graham Percival (old account)
I've only reviewed the first file. I don't think that reviewing more would do good; ...
3 years, 11 months ago #11
c_sorensen_byu.edu
On 5/5/10 12:01 PM, "Graham Percival" <graham@percival-music.ca> wrote: > On Wed, May 5, 2010 at ...
3 years, 11 months ago #12
Graham Percival
On Wed, May 05, 2010 at 06:04:42PM -0600, Carl Sorensen wrote: > > On 5/5/10 ...
3 years, 11 months ago #13
c_sorensen_byu.edu
On 5/5/10 6:11 PM, "Graham Percival" <graham@percival-music.ca> wrote: > On Wed, May 05, 2010 at ...
3 years, 11 months ago #14
Graham Percival
On Wed, May 05, 2010 at 06:31:21PM -0600, Carl Sorensen wrote: > On 5/5/10 6:11 ...
3 years, 11 months ago #15
markpolesky_yahoo.com
Patch set 3 uploaded. http://codereview.appspot.com/1056041 Hope this isn't driving you crazy. - Mark
3 years, 11 months ago #16
Trevor Daniels
Mark I've not examined every change in detail, but I think I've picked out all ...
3 years, 11 months ago #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 ...
3 years, 11 months ago #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: > ...
3 years, 11 months ago #19
Mark Polesky
Okay, this better be good enough to push; it's starting to get tiresome. Thanks for ...
3 years, 11 months ago #20
markpolesky_yahoo.com
patch set 4 is up: http://codereview.appspot.com/1056041
3 years, 11 months ago #21
markpolesky_yahoo.com
patch set 4 is up: http://codereview.appspot.com/1056041
3 years, 11 months ago #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): ...
3 years, 11 months ago #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 ...
3 years, 11 months ago #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 ...
3 years, 11 months ago #25
Graham Percival
3 years, 11 months ago #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 1274:9b9295c7fd64