jperon / lyluatex

Alternative à lilypond-book pour lualatex
MIT License
58 stars 11 forks source link

Regression on option parsing #156

Closed uliska closed 6 years ago

uliska commented 6 years ago

print-only={3,1,6} triggers

Module lyluatex Error: Unknown option: 1 on input line 29

Obviously the commas are not protected through the braces, I guess this has been introduced by the refactoring.

uliska commented 6 years ago

Another observation: rmfamily={Linux Libertine O} does not result in Linux Libertine O as option value but in {Linux Libertine O}

uliska commented 6 years ago

It turned out that in fact the options string was first split by the commas, which broke any values wrapped in braces.

I'm not sure if the code in https://github.com/jperon/lyluatex/commit/e56a80300164431f1ef138c45e3f8d94fd6327c5 could be more concise, but at least it seems to properly parse the option string.

uliska commented 6 years ago

It seems the fix still wasn't sufficient. While comma-separated values now are parsed properly rmfamily={Linux Libertine O} now triggers

lyluatex/lyluatex.lua:1371: attempt to concatenate local '
vs' (a nil value)
stack traceback:
    ...e/uliska/texmf/tex/latex/latex-git/lyluatex/lyluatex.lua:1371: in function 
'set_local_options'
    ...e/uliska/texmf/tex/latex/latex-git/lyluatex/lyluatex.lua:1299: in function 
'fragment'
    [\directlua]:1: in main chunk.
\ly@compilescore ... #1 ly.newpage_if_fullpage() }
                                                  \ly@resetunits \ly@current...

l.7 e' f' g' a' }

? 
uliska commented 6 years ago

I have looked into it, and I have the impression that the idea with the iterator local next_opt = opts:gmatch('([^,]+)') is just not going to work because we need an iterator that for the next option returns either the text up to the next comma or the next closing brace. And if I've learned correctly Lua's regex implementation doesn't support expressions like gmatch('([^,]+|[^}]+)').

If that's correct then I think we'd need something more similar to what I tried earlier: have a regular function that determines the next option more reliably and returns it together with the remainder of the opt string.

jperon commented 6 years ago

The iterator alone isn't sufficient (that's why I didn't write a simple for loop), but the repeat… until loop takes care of the braces. I've nothing against the implementation in #158, but as the fix was just changing one line and the result is slightly more optimized, I pushed another solution on 15cd4b933e4c14de6bcbf87f63f34af54747cb5f.

uliska commented 6 years ago

Thank you, I always prefer clean and easy solutions ;-)

uliska commented 6 years ago

It still doesn't work properly when a braced option without comma in it is followed by another option:

print-only={1,3,5},debug works but

rmfamily={LInux Libertine O},debug does not.

Debug printing reveals that the code will swallow all comma-separated items until the end of the string or until a "sub-option" ends with a brace (which will never happen if the braced value doesn't have commas:

[rmfamily={TeXGyre Adventor},debug,current-font-as-main] 
=>
Determined options
Key: rmfamily
Value: TeXGyre Adventor},debug,current-font-as-mai

[debug,rmfamily={TeXGyre Adventor},current-font-as-main]
=>
Determined options
Key: debug
Value: 
Key: rmfamily
Value: TeXGyre Adventor},current-font-as-mai
jperon commented 6 years ago

Fixed in #161.

jperon commented 6 years ago

Just a side note while I think about it: with the new option parsing, it's not necessary to wrap an unique value between braces, even when there are spaces. For example, rmfamily=Linux Libertine O, debug works.