Reorganize language files and add a new \language command.

Reorganize language files and add a new \language command.

This commit adds a new \language command that may be used
to select note names language in lieu of the more complicated
\include "foobar.ly" syntax.

All language files have been emptied and the pitchnames alists
have been gathered in a new language-init.ly file.

The old syntax is still supported; to avoid code duplication,
language files just use the new \language command internally.

arabic.ly isn't affected by this change and must still be
manually included.  This specificity will have to be taken
into account when documenting the new syntax.

[Valentin Villenave, 2010-10-25 00:37:00]

Greetings everybody.

Third time's a charm: here's a new attempt to get the \language "foobar" syntax
merged. Cons: this patch does move a lot of files around. Pros: this is a lot
less hackish that my two previous attempts.

Please have a look and feel free to comment; if this patch is approved, then
I'll upload a second patch set to handle Documentation changes!

Thanks to Neil and Carl for their comments and suggestions. Hopefully this time
I got it right... :-)

[Carl, 2010-10-25 01:57:56]

I like this a lot -- it's looking like real lilypond code, instead of a hack.

Thanks for doing this!

Carl

http://codereview.appspot.com/2699041/diff/1/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/2699041/diff/1/Documentation/changes.tely#newco...
Documentation/changes.tely:74: is still supported, but might be deprecated in
the future.
Don't forget to add that the new syntax is compatible with safe mode (at least I
think it is).

http://codereview.appspot.com/2699041/diff/1/ly/catalan.ly
File ly/catalan.ly (right):

http://codereview.appspot.com/2699041/diff/1/ly/catalan.ly#newcode4
ly/catalan.ly:4: \version "2.12.0"
Update the \version string in all of these files!

http://codereview.appspot.com/2699041/diff/3001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2699041/diff/3001/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:365: (_i "Select note names language.")
It would be more like standard lilypond to have language-init.ly do something
like:

language-pitch-names = 
`((nederlands . ((ceses . ,(ly:make-pitch -1 0 DOUBLE-FLAT))
                          (ceh . ,(ly:make-pitch -1 0 SEMI-FLAT))
             ...
                         ))
   (italiano . ((....)
                   ...
                   ))
   (english .    ...
                  ))
    ....
  )
so that you have a single alist.

Then, the argument to your music function would not be a string, but rather a
symbol:

\language #'italiano

And the definition of language becomes:

#(define-music-function (parser location lang) (symbol?)
...
(let ((alist-def (assoc-get lang language-pitch-names '())))
 ...  (the rest of your stuff is the same)


That way we don't have to mess with strings and string comparisons, we just use
symbols.

[Valentin Villenave, 2010-10-25 02:17:16]

On Mon, Oct 25, 2010 at 3:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
> Update the \version string in all of these files!

Well, I was reluctant to put a minor version number. Do you think
putting "2.14.0" would be okay?

> Then, the argument to your music function would not be a string, but
> rather a symbol:
>
> \language #'italiano

I did consider it. However, I have three reasons not to use symbols:
- from a user perspective, typing "italiano" is slightly more
straightforward than typing #'italiano. Remember, this function is
intended for newcomers, who wouldn't even use music variables or
\score blocks, let alone \overrides, boolean or symbols!
- this also allows me to treat the given argument as case-insensitive.
- last but not least, if we ever do implement the \language command as
a parser token, taking a simple-string argument, then almost no
changes will be needed (if anything, the "foobar" syntax will still
work, but plain \language foobar, without quotes, will also be
possible).

That being said, I can indeed nest all the alists together, without
necessarily using a (symbol?) argument: string->symbol is reliable
enough A new patchset is on its way..

> That way we don't have to mess with strings and string comparisons, we
> just use symbols.

Well, I hope I made my point about using strings. I'm pretty hopeful
that string->symbol does a reliable enough of a job to not qualify as
"messy" :-)

[c_sorensen, 2010-10-25 04:16:09]

On 10/24/10 8:17 PM, "Valentin Villenave" <v.villenave@gmail.com> wrote:

> On Mon, Oct 25, 2010 at 3:57 AM,  <Carl.D.Sorensen@gmail.com> wrote:
>> Update the \version string in all of these files!
> 
> Well, I was reluctant to put a minor version number. Do you think
> putting "2.14.0" would be okay?
> 
>> Then, the argument to your music function would not be a string, but
>> rather a symbol:
>> 
>> \language #'italiano
> 
> I did consider it. However, I have three reasons not to use symbols:
> - from a user perspective, typing "italiano" is slightly more
> straightforward than typing #'italiano. Remember, this function is
> intended for newcomers, who wouldn't even use music variables or
> \score blocks, let alone \overrides, boolean or symbols!

Well, although your intention is to make it easy for newcomers, that's not
(IMO) a good reason to move forward on this.  There are lots of other ways
to make it easy on newcomers (e.g. a template file that starts with the
language statement).  For me, the real reason is to get a language selection
method that can run in safe mode.

> - this also allows me to treat the given argument as case-insensitive.

I think this is a reason *not* to use the case-insensitive implementation.
LilyPond input *is* case sensitive (r1 is not the same as R1, c4 is not the
same as C4).  I think we should always be case-sensitive if we're ever
case-sensitive.  I would prefer to eliminate the case conversion part of the
patch.  I think that the best thing we can do to be easier is to be
consistent.

> - last but not least, if we ever do implement the \language command as
> a parser token, taking a simple-string argument, then almost no
> changes will be needed (if anything, the "foobar" syntax will still
> work, but plain \language foobar, without quotes, will also be
> possible).

This answer is much more compelling to me.  \language italiano (like \clef
treble) is very consistent.  So you've convinced me that a string argument
is OK.

> 
> That being said, I can indeed nest all the alists together, without
> necessarily using a (symbol?) argument: string->symbol is reliable
> enough A new patchset is on its way..

Perfect!

> 
>> That way we don't have to mess with strings and string comparisons, we
>> just use symbols.
> 
> Well, I hope I made my point about using strings. I'm pretty hopeful
> that string->symbol does a reliable enough of a job to not qualify as
> "messy" :-)

Yep, it's fine.

Thanks,

Carl

[Patrick McCarty, 2010-10-25 04:36:31]

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.ly
File input/regression/note-names.ly (right):

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:8: \version "2.13.37"
I prefer having \version statements at the top of regression tests, but it's up
to you.

http://codereview.appspot.com/2699041/diff/9001/ly/catalan.ly
File ly/catalan.ly (right):

http://codereview.appspot.com/2699041/diff/9001/ly/catalan.ly#newcode2
ly/catalan.ly:2: %%%% (see language-init.ly)
Even though you are making these legacy files, I would not remove copyright
information from any of them.

Jan made a big effort adding all of these copyright statements to comply closer
with GNU standards...

http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly
File ly/language-init.ly (right):

http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly#newcode23
ly/language-init.ly:23: ;  Language: Nederlands
--------------------------------------------;
Scheme code comments traditionally use two semicolons, that is

;; Language: etc.

http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly#newcode25
ly/language-init.ly:25: ;   Copyright (C) 1996--2010 Han-Wen Nienhuys
<hanwen@xs4all.nl>
IMO, the code starts to look cluttered seeing all of these copyright statements
spread throughout the file.

I would put them all in the file header.

[wl_gnu.org, 2010-10-25 05:24:31]

> \language italiano (like \clef treble) is very consistent.  So
> you've convinced me that a string argument is OK.

Mhmm, in the documentation, please use

  \language "italiano"

instead.  Even newcomers must get used to it...


    Werner

[Graham Percival (old account), 2010-10-25 08:05:16]

It's not _the_ charm, but it's certainly much more charming.  Calm down, take
your time, and make it beautiful.  I'm not going to start the 2.14 release
candidate without this patch, so there's no rush.

Expect to do 1-3 more candidate patches before it's finally accepted.

http://codereview.appspot.com/2699041/diff/9001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/2699041/diff/9001/Documentation/changes.tely#ne...
Documentation/changes.tely:75: but might be deprecated in the future.
Not might; will.  I'd also add a sentence saying that for this purpose,
arabic.ly counts as a musical style rather than a language.

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.ly
File input/regression/note-names.ly (right):

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:8: \version "2.13.37"
On 2010/10/25 04:36:31, Patrick McCarty wrote:
> I prefer having \version statements at the top of regression tests, but it's
up
> to you.

+1

[Trevor Daniels, 2010-10-25 08:39:06]

Congratulations, Valentin, you're on to a winner now!

I've no comments to add to those already made.

Trevor

http://codereview.appspot.com/2699041/diff/9001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/2699041/diff/9001/Documentation/changes.tely#ne...
Documentation/changes.tely:75: but might be deprecated in the future.
On 2010/10/25 08:05:16, Graham Percival wrote:

Done.

[pacovila, 2010-10-25 13:06:24]

2010/10/25  <v.villenave@gmail.com>:

> Third time's a charm

Congrats!

-- 
Francisco Vila. Badajoz (Spain)
www.paconet.org , www.csmbadajoz.com

[Neil Puttock, 2010-10-25 22:26:29]

Hi Valentin,

I think this a great improvement on the last patch.  Once you've sorted out the
nitpicks, it should be good to go.

Cheers,
Neil

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.ly
File input/regression/note-names.ly (right):

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:15: g bf d c
g4

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:20: (set! pitchnames dutchPitchnames)
Heh, you still haven't tested this, have you?

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:22: bes a g fis
bes4

http://codereview.appspot.com/2699041/diff/9001/input/regression/note-names.l...
input/regression/note-names.ly:26: sol fa mib re
sol4

http://codereview.appspot.com/2699041/diff/9001/ly/catalan.ly
File ly/catalan.ly (right):

http://codereview.appspot.com/2699041/diff/9001/ly/catalan.ly#newcode6
ly/catalan.ly:6: \language "catalan"
add a newline

(ditto all other language files)

http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly
File ly/language-init.ly (right):

http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly#newcode22
ly/language-init.ly:22: #(define language-pitch-names `(
needs indenting correctly (I suggest moving `( to the next line, otherwise the
correct indentation will be too far to the right)

http://codereview.appspot.com/2699041/diff/9001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2699041/diff/9001/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:368: (if (not (null? language-def))
I prefer

(if (pair? language-def)

http://codereview.appspot.com/2699041/diff/9001/ly/music-functions-init.ly#ne...
ly/music-functions-init.ly:374: (ly:warning (_ "Could not find language ~a.
Ignoring.")  str))
remove extra space before `str'

[Valentin Villenave, 2010-10-27 00:48:09]

Greetings everybody,

new patch set. Please have a look!

\version statements: I have put 2.13.38 in the regtest, but 2.14.0 in the .ly
init files. Putting a minor version number in these files just didn't feel
right.  And 2.14 is near, isn't it? (Color me optimistic. :-)

The only change that I didn't make was:

On 2010/10/25 04:36:31, Patrick McCarty wrote:
> http://codereview.appspot.com/2699041/diff/9001/ly/language-init.ly#newcode25
> IMO, the code starts to look cluttered seeing all of these copyright
statements
> spread throughout the file.
> I would put them all in the file header.

Well, these aren't just copyright statements. These are the pitch-maps that were
previously given as block-comments in foobar.ly language files. I merely added
one-line copyright statements to give proper attribution (although I have now
also restored full GNU headers in deprecated .ly files).  TBH, I am not sure
either that these commented chunks are that useful; however, there's little harm
in keeping them for now.

Anyway. Now I hope that we're getting somewhere :)

Cheers.

[Carl, 2010-10-27 03:17:24]

On 2010/10/27 00:48:09, Valentin Villenave wrote:
> Greetings everybody,
> 
> new patch set. Please have a look!
> 
> \version statements: I have put 2.13.38 in the regtest, but 2.14.0 in the .ly
> init files. Putting a minor version number in these files just didn't feel
> right.  And 2.14 is near, isn't it? (Color me optimistic. :-)

We need to have 2.13.38, so that the automatic update will work properly.

[Carl, 2010-10-27 03:23:02]

Looks good!

See my comments on the UTF-8 characters in the names getting mangled.

Thanks,

Carl

http://codereview.appspot.com/2699041/diff/24001/ly/norsk.ly
File ly/norsk.ly (right):

http://codereview.appspot.com/2699041/diff/24001/ly/norsk.ly#newcode4
ly/norsk.ly:4: %%%% Copyright (C) 1998--2010 Arvid Grøtting <arvidg@ifi.uio.no>
Looks like the UTF-8 got mangled here.

I think this should say

Copyright (C) 2010 Valentin Villenave

since none of the code remaining in this file belongs to Arvid.

And I think  that you should add all of the individual file authors to 
the formal statement at the top of language-init.ly.

And I agree with keeping the authors in the block comments for each language --
but maybe not the copyright statement (since it's at the top of the file).

[Graham Percival (old account), 2010-10-27 07:08:19]

Looks mostly good.

http://codereview.appspot.com/2699041/diff/24001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/2699041/diff/24001/Documentation/changes.tely#n...
Documentation/changes.tely:74: be used in safe mode).  The old syntax is still
supported,
That sentence has a "clause: clause, clause (clause)".  This seems a bit
convoluted.  How about:

Note names can be selected with a new @code{\language "italiano"} command, which
can be used in safe mode.  The old syntax is still supported for now, but will
be deprecated in the future.

http://codereview.appspot.com/2699041/diff/24001/ly/catalan.ly
File ly/catalan.ly (right):

http://codereview.appspot.com/2699041/diff/24001/ly/catalan.ly#newcode21
ly/catalan.ly:21: \version "2.14.0"
Won't this break compiling?  I'd expect lilypond to quit with a "file too new;
update your version of lilypond" message.

Make it 2.13.37.

[Valentin Villenave, 2010-10-27 10:08:33]

Greetings everybody,

updated patch set.

Something keeps removing traling newlines at the end of files, I'm not
sure what does that (I have configured git core.whitespace properly,
though: could git cl or codereview be the culprit?).

On Wed, Oct 27, 2010 at 9:08 AM,  <percival.music.ca@gmail.com> wrote:
> That sentence has a "clause: clause, clause (clause)".  This seems a bit
> convoluted.  How about:
>
> Note names can be selected with a new @code{\language "italiano"}
> command, which can be used in safe mode.  The old syntax is still
> supported for now, but will be deprecated in the future.

Thanks, that's better indeed. (In case you hadn't noticed, I do have a
thing for long sentences with lots of commas :)

> Won't this break compiling?  I'd expect lilypond to quit with a "file
> too new; update your version of lilypond" message.

It doesn't quit, but it does complain.

> Make it 2.13.37.

Done.

Carl, thanks for your advice. I have updated all copyright notices
accordingly to what you suggested.

Cheers,
Valentin.

[Graham Percival (old account), 2010-10-28 03:41:39]

LGTM

[Patrick McCarty, 2010-10-28 05:15:57]

Looks pretty good to me.  Just a tiny style nitpick indicated below.

Thanks for your work on this!

Regards,
Patrick

http://codereview.appspot.com/2699041/diff/33001/input/regression/note-names.ly
File input/regression/note-names.ly (right):

http://codereview.appspot.com/2699041/diff/33001/input/regression/note-names....
input/regression/note-names.ly:20: (set! pitchnames(ly:assoc-get 'nederlands
language-pitch-names))
There should be a space between "pitchnames" and "(ly:assoc-get"

[Valentin Villenave, 2010-10-28 09:37:25]

On Thu, Oct 28, 2010 at 7:15 AM,  <pnorcks@gmail.com> wrote:
> There should be a space between "pitchnames" and "(ly:assoc-get"

Wow, nice catch!

Pfew. Pushed and (hopefully) closed.
Thanks everybody; now comes the documentation work!

Cheers,
Valentin.