Handle magic insertion marks when merging external features #1259
Conversation
cmyr
force-pushed
the
insert-features
branch
from
502ebea to
86997c3
Compare
| assert!(item | ||
| .token_text() | ||
| .unwrap_or_default() | ||
| .contains("# Automatic Code")); |
There was a problem hiding this comment.
I don't love the assert, shouldn't we just check if it matches and go about our business if it doesn't? Is it imposible to write some other comment in this position for some reason?
There was a problem hiding this comment.
We filter out comments as a rule, but we have an exception for comments containing this string. If this is ever not true, something is seriously broken.
fea-rs/src/compile/compile_ctx.rs
Outdated
| assert!( | ||
| _name.is_none(), | ||
| "named lookup should be finished before insert marker" | ||
| ); |
There was a problem hiding this comment.
If this is a bad state lets error. If it's not lets warn. Either way lets not assert.
There was a problem hiding this comment.
I was thinking this is an invariant but actually this could occur in the wild if someone wrote,
feature test {
lookup oops {
# Automatic Code
} oops;
} test;that is, they put the comment inside a lookup block inside a feature.
Updated to just error in this case.
That is, we want to respect the '# Automatic Code' comments that are used in glyphs sources. Previously we would always just append any generated looups at the back of the list; now if these markers are present we will insert the lookups at that position.
cmyr
force-pushed
the
insert-features
branch
from
86997c3 to
a2baa71
Compare
This was about as straight-forward as I could've hoped for, I think, and gives us a modest crater boost.