Re: Create a two-argument form of define-event-class (issue 10965043)

[thomasmorley65]

On 2013/07/06 00:05:35, dak wrote:

https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm
> File scm/define-event-classes.scm (right):


https://codereview.appspot.com/10965043/diff/1/scm/define-event-classes.scm#newcode87
> scm/define-event-classes.scm:87: (define ancestor-lookup
(make-hash-table
> (length all-event-classes)))
> AOn 2013/07/05 23:20:48, thomasmorley651 wrote:
> > I'm not convinced about the naming.
> >
> > 'ancestor' is defined in titling-init.ly, ofcourse with different
> functionality.
> > 'ancestor-lookup' is a _very_ generic naming, and it's usage _is_
generic in a
> > certain way.
> > Though, I'd prefer something like
'ancestor-all-event-classes-lookup'.
> > No, that's to long.
> > But you get the point.

> Actually, I don't get the point.  This is not a public variable, so
there is not
> much chance for collision.  It's local to the lily module, and come
Guilev2, it
> will be local to this file.

True. Ofcourse.
It's more that I'd prefer namings which mirrors more exactly what the
defined stuff will do.
(Though, that would mean to rename a the definition in titling-init.ly,
too. And probably a lot of others throughout our source-code.)

> https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm
> File scm/document-music.scm (right):


https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode33
> scm/document-music.scm:33: (let* ((class
(ly:camel-case->lisp-identifier (car
> entry)))
> On 2013/07/05 23:20:48, thomasmorley651 wrote:
> > Apart from using tabs I'm not able to see any difference to the old
file.

> The call to ly:make-event-class takes one argument less, and
doc-context is no
> longer defined.

Ahh, I've overlooked it.

> > No offense, just curious about it:
> > I was told not to use tabs, what's the reason for using them here?

> The first commit in the series is a revert.  It brought the tabs back
from the
> dead: apparently I untabified as part of the commit in question.  I
can probably
> rerun the Scheme indenter.  The "revert" commit is not a clean revert
anyway.

Thanks for your explication.


https://codereview.appspot.com/10965043/diff/1/scm/document-music.scm#newcode91
> scm/document-music.scm:91: (classes (ly:make-event-class class))
> Again: this line has changed non-trivially, and the context around it
is then a
> large merge conflict because of having gotten untabified in the
original commit
> that has now been reverted.



https://codereview.appspot.com/10965043/