[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by add

From: Carl . D . Sorensen
Subject: Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by address@hidden)
Date: Mon, 01 Apr 2019 21:14:48 -0700

On 2018/11/10 05:44:23, pwm wrote:
Documentation/notation/chords.itely:679: represent the structure of
the chord.
When entered in node mode,
typo: "note mode"
input/regression/ texidoc = "The property
@code{chordNameExceptions} can used
'can be used' (This was carried over, but might as well fix while
we're here.)

input/regression/ chExceptions = #(append
(chordmode->exception-entry chordVar markupVar) chExceptions)
Hmm, chExceptions is used in its own definition here?  Does that work?
 This is
not making sense to me.

chExceptions is previously defined to be a couple of new chords
prepended to ignatzekExceptions, so it can be used this way.
Whitespace on unnecessary blank line.

input/regression/ texidoc = "The
@code{chordSemanticsNameExceptions} can used
can be used

Whitespace on unneeded blank line.

Unnecessary new line.

scm/chord-entry.scm:75: chord-semantics))
It's hard to read this code because of the way it's formatted.  Would
be better
with more line-breaks to keep the lines from being too long and the
from going so far to the right and wrapping around to the next line
(in narrow
views like this code review one).  Similar comment for other places in
this file
like this.

scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
Hmm, so there's a single 'chord-semantics property under a
?  Seems like we could avoid that extra step of nesting and just have
subproperties of 'chord-semantics under 'ChordSemanticsEvent ?  And
that would
be more like NoteEvent and other events.  Or maybe I'm missing

We could do that, but it would pollute the global namespace with all the
that are part of 'chord-semantics and are only used for chord semantics.
 By putting them
in a single property, we avoid polluting the namespace.  Similar to the
way we do fret-diagram-details.
scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
Would be more clear and consistent if original-inv-pitch were renamed
original-inv-entry or similar.

Why?  Not to be argumentative, but I wonder what about this name is more
clear and consistent, in your opinion?
scm/chord-entry.scm:267: (append (if bass (write-me "base3: " bass)
(make-note-ev bass 'bass #t)) '())
This write-me looks like a debugging statement that was left in?

scm/chord-entry.scm:272: (bass (make-elements (cons (make-note-ev bass
(cons #t #t))
Hmm, this (cons #t #t) looks like it could be related to one of the
test failures that James posted about?

Will look more later.
scm/chord-entry.scm:306: (assoc-ref semantics-list key))
This is defined twice, see line 292 above.

scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)))))
Instead of all of these (set! quality ...) why not just let the result
of this
cond be assigned to 'quality' and add an 'else' 'maj at the end?
(quality (cond ((= alteration SHARP) 'aug) ...))

I think it's because for some intervals, you set a zero alteration as
perfect, while
for others, the zero alteration is 'maj.  So it doesn't drop easily into
an else, I think.
scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
Defining quality using cond or case would be a more idiomatic
Scheme-ish way to
do it, avoiding the mutation of set!.

I totally agree with you; I'll try to work this out for a future patch.
For now, I want
to just get it into review.
scm/chord-entry.scm:414: (cons (list (cons 'step-number step-number)
'step-quality quality)) chord-step-list)
Why not use make-chord-step here to simplify this?  Also, I'd wrap
this to more
than one line, to break it up and make it easier to read and

scm/chord-entry.scm:425: (make-chord-entry (ly:make-pitch 0 (- n 1)
(nca n))
Here's a way to simplify this:

  (map (lambda (chord-step)
         (let* ((sn (assoc-ref chord-step 'step-number))
                (octave (if (>= sn 8) 1 0))
                (note (- sn (if (>= sn 8) 8 1)))
                (alter (if (= sn 7) FLAT 0))
                (pitch (ly:make-pitch octave note alter)))
           (make-chord-entry pitch chord-step)))

scm/chord-ignatzek-names.scm:44: "Does PS have the X step? Return that
step if
it does."
Usually Scheme doc strings come after the 'define' like the one in
double quotes
here.  I think it would be better to do them that way, rather than
with ';;'
above the 'define'.  That will allow consolidation of some of these
that have
both here.  Also, ps is short for pitches, so it would be clearer to
rename with
ces, es, or ch-es (or something) for chord entries here.
scm/chord-ignatzek-names.scm:62: "Copy PS, but replace the step of P
in PS."
ps -> es, ces, ch-es or something here as well. Probably p -> e too.
elsewhere as needed.

scm/chord-ignatzek-names.scm:317: lowercase-root?))))))
Indentation seems off here, compared to before this patch.

scm/chord-ignatzek-names.scm:362: empty-markup))
These make-removals-markup and make-additions-markup functions are
very similar.
  If you are feeling motivated, it would be good to refactor to remove

Save for next review
Whitespace to remove.
scm/chord-ignatzek-names.scm:395: (append
Trailing whitespace nit.

Should be cleared up.
scm/chord-ignatzek-names.scm:405: (ly:context-property context
Formatting, line breaking, indentation causing this code to go too far
to the

scm/chord-name.scm:177: (filter not-root-entry? semantics-list))
Could use a blank line between these two define-publics.

scm/chord-name.scm:181: ;; chordmode->exception-entry
This comment doesn't add much.

scm/chord-name.scm:185: "
Closing " usually doesn't get its own line.
scm/chord-name.scm:187: (define (get-semantics-event music)
Optional, but it's probably clearer to move this define up a level and
not nest
it under the other defines so deeply.

scm/chord-name.scm:204: return))
The 'return' is not adding much here.  I'd suggest dropping it and
just having
the result of the cond be the return value for the function, without
the (set!
return ...) expressions, and with an else '() fallback.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]