Menu
▾
▴
#3815 Patch: Parser: harmonize \lyricsto and \addlyrics arguments
Originally created by: *anonymous
Originally created by: dak@gnu.org
Originally owned by: dak@gnu.org
Parser: harmonize \lyricsto and \addlyrics arguments
Previously, they allowed arbitrary music but since that required looking
at lookahead tokens in the wrong mode, it led to strange errors like with
lyr=\lyricsto A { }
mus={c}
when mus= was parsed as a single token in lyrics mode. Now both \addlyrics
and \lyricsto accept merely the same kind of delimited argument list
that \lyrics or \chords accept. Additionally, to preserve compatibility
to a lot of examples, music identifiers like \mus are permitted as the
argument to \lyricsto and \addlyrics.
Originally posted by: dak@gnu.org
(No comment was entered for this change.)
Owner: dak@gnu.org
Originally posted by: dak@gnu.org
Patchy the autobot says: Impressive number of failing files. A number of them might want addressing with a convert-ly rule, but first one needs to figure out what kind of usage actually works and what is redundant. Current list looks to be "lyric-combine-empty-warning.ly","one-line-breaking.ly","profile-property-access.ly","lyric-combine-new.ly","song-melisma.ly","hara-kiri-stanza-number.ly","page-spacing-nonstaff-lines-and-markup.ly","lyric-extender-broken.ly","lyric-phrasing.ly","typography-demo.ly","page-spacing-nonstaff-lines-independent.ly","song-repetition.ly","lyrics-melisma-beam.ly","morgenlied.ly","lyric-combine-polyphonic.ly","span-bar-allow-span-bar.ly","lyrics-after-grace.ly","midi-lyric-barcheck.ly","lyric-extender-completion.ly",
Labels: -Patch-new Patch-needs_work
Originally posted by: pkx1...@gmail.com
jlowe@jlowe26vm /tmp/show-3815$ more ./out/lybook-testdb/e9/lily-cc1cc235.log
Processing `/tmp/build-lilypond-autobuild/out/lybook-testdb/e9/lily-cc1cc235.ly'
Parsing...
Renaming input to: `/tmp/lilypond-autobuild/input/regression/lyric-combine-polyphonic.ly'
/tmp/lilypond-autobuild/input/regression/lyric-combine-polyphonic.ly:24:24: error: syntax error, unexpected \context, expecting \
sequential or \simultaneous or << or '{'
\lyricsto "one"
\context Lyrics = sop {
/tmp/lilypond-autobuild/input/regression/lyric-combine-polyphonic.ly:31:24: error: syntax error, unexpected \new, expecting \sequ
ential or \simultaneous or << or '{'
\lyricsto "two"
\new Lyrics {
/tmp/lilypond-autobuild/input/regression/lyric-combine-polyphonic.ly:46:1: error: Unfinished main input
% ****************************************************************
/tmp/lilypond-autobuild/input/regression/lyric-combine-polyphonic.ly:11:1: error: errors found, ignoring music expression
{
Writing timing to e9/lily-cc1cc235.profile...
Originally posted by: dak@gnu.org
convert-ly rules for harmonizing \lyricsto usage
http://codereview.appspot.com/53120044
Labels: -Patch-needs_work Patch-new
Originally posted by: pkx1...@gmail.com
Still fails make check although on less files
--snip--
jlowe@jlowe26vm /tmp/show-3815$ more ./out/lybook-testdb/16/lily-fb4bd50d.log
Processing `/tmp/build-lilypond-autobuild/out/lybook-testdb/16/lily-fb4bd50d.ly'
Parsing...
Renaming input to: `/tmp/lilypond-autobuild/input/regression/one-line-breaking.ly'
/tmp/lilypond-autobuild/input/regression/typography-demo.ly:130:29: error: syntax error, unexpected \new, expecting \sequential or \simultaneous or << or '{'
\lyricsto "singer"
\new Lyrics \firstVerse
/tmp/lilypond-autobuild/input/regression/typography-demo.ly:131:29: error: syntax error, unexpected \new, expecting \sequential or \simultaneous or << or '{'
\lyricsto "singer"
\new Lyrics \secondVerse
/tmp/lilypond-autobuild/input/regression/typography-demo.ly:123:7: error: errors found, ignoring music expression
<< \time 6/8
/tmp/lilypond-autobuild/input/regression/one-line-breaking.ly:15:1: error: Unfinished main input
% ****************************************************************
warning: 1 expected warning(s) not encountered:
(De)crescendo with unspecified starting volume in MIDI.
Writing timing to 16/lily-fb4bd50d.profile...
--snip--
% ****************************************************************
% ly snippet:
% ****************************************************************
\sourcefilename "/tmp/lilypond-autobuild/input/regression/one-line-breaking.ly"
\sourcefileline 0
\version "2.16.0"
\header {
texidoc = "The @var{ly:one-line-breaking} algorithm puts everything on one line."
}
\paper { page-breaking = #ly:one-line-breaking }
\include "typography-demo.ly"
% ****************************************************************
% end ly snippet
% ****************************************************************
Labels: -Patch-new Patch-needs_work
Originally posted by: dak@gnu.org
Patchy the autobot says: Still a few badies
Originally posted by: dak@gnu.org
Further pattern fixes
http://codereview.appspot.com/53120044
Labels: -Patch-needs_work Patch-new
Originally posted by: dak@gnu.org
Patchy the autobot says: passes tests. But since this is restraining syntax and regularizing usage, docs need to get tested as well.
Labels: -Patch-new Patch-review
Originally posted by: dak@gnu.org
This is actually reining in \lyricsto rather heavily. Basically, you can use it like \lyricmode: putting context creation commands or music functions right behind it without additional braces is not possible any more. Since the bleedover of lyric mode has ugly consequences, and since more permissive closed subsets will only facilitate "I don't know what exactly I can use after \lyricsto" confusion, I think that basically clamping down the permissible permutations to a subset working with other mode-changing commands makes sense.
It would probably make sense to follow this up with tightening the documentation accordingly.
I'm giving this a "Maintainability" tag. While it has been triggered by a bug report, the bug is a fringe consequence of \lyricsto's previous behavior.
Labels: -Type-Enhancement Type-Maintainability
Originally posted by: pkx1...@gmail.com
Patch on countdown for Jan 21st - 06:00 GMT
Labels: -Patch-review Patch-countdown
Originally posted by: pkx1...@gmail.com
Patch counted down - please push.
NB: Comment #8 asks about a Doc test. If you still need this then set it to patch-new and I'll pick it up and test it.
Labels: -Patch-countdown Patch-push
Originally posted by: dak@gnu.org
Oh, I don't need a separate doc test _now_ that the countdown ended: I'll just push to staging and let Patchy test. If it doesn't work, this will need another review cycle. Pushed to staging as a merge commit (as it includes uncompilable tests before running convert-ly):
* commit [r809c04e37b8d3d368eda7e4b576e0d80d1934cc3]
|\ Merge: 9d481c2 9fb28f4
| | Author: David Kastrup <dak@gnu.org>
| | Date: Tue Jan 21 13:30:02 2014 +0100
| |
| | Merge branch 'issue3815' into staging
| |
| * commit [r9fb28f4db2cf0f4d302f70c4def238cd2145f329]
| | Author: David Kastrup <dak@gnu.org>
| | Date: Tue Jan 21 13:27:45 2014 +0100
| |
| | Run scripts/auxiliar/update-with-convert-ly.sh
| |
| * commit [r940767af99f39411e3e519a75968be11d47e3083]
|/ Author: David Kastrup <dak@gnu.org>
| Date: Thu Jan 16 11:25:23 2014 +0100
|
| Issue 3815: Parser: harmonize \lyricsto and \addlyrics arguments
|
| Previously, they allowed arbitrary music but since that required looking
| at lookahead tokens in the wrong mode, it led to strange errors like with
| lyr=\lyricsto A { }
| mus={c}
| when mus= was parsed as a single token in lyrics mode. Now both \addlyrics
| and \lyricsto accept merely the same kind of delimited argument list
| that \lyrics or \chords accept. Additionally, to preserve compatibility
| to a lot of examples, music identifiers like \mus are permitted as the
| argument to \lyricsto and \addlyrics.
|
| Since this requires a change in existing uses, this includes a
| convert-ly rule that removes redundant uses of \lyricmode and
| rearranges the combination with context starters such that \lyricsto
| in general is applied last, like \lyricmode would be.
|
Labels: -Patch-push Fixed_2_19_2
Status: Fixed
Originally posted by: Elu...@gmail.com
(No comment was entered for this change.)
Status: Verified