-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add child combinator ">" (and fix a specificity bug) #233
Conversation
Bug: Just because a trie element exists for a more specific scope doesn‘t mean its parent scopes will match, so we need to collect the trie elements with less specific scopes too. Bug: If the number of scope names in both rules‘ scope paths are not equal, the parent scope names won‘t be compared at all. Instead, the rule with the longest scope path is preferred. This goes against the TextMate manual (https://macromates.com/manual/en/scope_selectors). In particular, the following line in “Ranking Matches”: > Rules 1 and 2 applied again to the scope selector when removing the deepest element (in the case of a tie) Feature: Add support for the child combinator (the `>` operator). This allows for styling a parent-child relationship specifically.
@hediet Excuse me if it's rude to tag, but I saw you're the only person to touch Test cases incoming... |
23e9193
to
0e71e37
Compare
- One for the new child combinator - One for bug 1 ("Theme resolving falls back to less specific rules") - One for bug 2 ("Theme resolving should give deeper scopes higher specificity")
After trying to reproduce the alleged bug, I realized it‘s not a bug. When a ThemeTrieElement is created, it inherits the `_rulesWithParentScopes` array of its parent element, which is guaranteed to be populated due to the lexicographical sorting of the theme rules in `resolveParsedThemeRules`.
…and update the other bug‘s test case to actually fail on the main branch
Okay tests have been added! Both fail on main branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good! Thank you! ❤️
This is ported from upstream vscode-textmate PR microsoft/vscode-textmate#233
This is ported from upstream vscode-textmate PR microsoft/vscode-textmate#233
This is ported from upstream vscode-textmate PR microsoft/vscode-textmate#233
Closes #123
Feature: Add support for the child combinator (the
>
operator). This allows for styling a parent-child relationship specifically.vscode-textmate/src/theme.ts
Lines 173 to 180 in d63ed16
vscode-textmate/src/theme.ts
Lines 526 to 532 in d63ed16
Bug: If the number of scope names in both rules‘ scope paths are not equal, the parent scope names won‘t be compared at all. Instead, the rule with the longest scope path is preferred. This goes against the TextMate manual (https://macromates.com/manual/en/scope_selectors). In particular, the following line in “Ranking Matches”:
vscode-textmate/src/theme.ts
Lines 506 to 515 in 09effd8
How did I fix it?
Do a depth-first, scope-by-scope comparison of the parent scopes, even if one of the rules has a longer scope path.
vscode-textmate/src/theme.ts
Lines 539 to 553 in 7c14889
Tests
vscode-textmate/src/tests/themes.test.ts
Line 609 in 7c14889
vscode-textmate/src/tests/themes.test.ts
Line 635 in 7c14889