-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update sublimehq/Packages to master #644
Conversation
Thank you very much for looking into this. Great that we can now actually upgrade the submodule to the latest state without getting any errors. Unfortunately, things are not that simple. The
After updating to the lastest version of sublimehq/Packages, there are quite a few more:
Having more test failures isn't necessarily a sign that things got worse (it could just be more tests), but I'm afraid it might very well be. I think this is also the reason why trishume/syntect#246 has not been merged. (also, after updating the /cc @keith-hall: FYI |
If i run |
some of the failing syntax test assertions are false positives due to syntect behaving differently than Sublime wrt how it handles assertions which span the end of the line, and would be fixed by trishume/syntect#185. I've just seen I need to fix some conflicts on that PR. |
Thank you both for the information! I was about to say "let's give this a try" when I ran {
"a": {
"b": 1
}
}
|
That is interesting, and definitely worth looking into - thanks for the minimal test case @sharkdp. I can't promise when I will find time to do so, however, so if someone else gets chance to investigate, please do! I wonder if it is related to the yaml-rust dependency, or the syntax definition changes. (presumably the latter but it would be nice to isolate to confirm) I believe the syntect unit test failures are just because they are brittle tests relying on some of the syntaxes from the sublimehq packages and color schemes which are also submodules and occasionally updated. Any change is likely to break the tests unfortunately. |
I can confirm that note to self:I had some fun getting my old copy of the `syntect` repo up to date.
cargo run --example syncat -- testdata/Packages/JavaScript/tests/syntax_test_json.json causes a panic, cargo run --example syntest -- testdata/Packages/JavaScript/tests/syntax_test_json.json shows some test failures. It may be related to clearing and restoring scopes
|
A new version of syntect has been released (trishume/syntect#259 (comment)) which includes the fix by @keith-hall. @benwaffle If possible, could you please update the PR and have bat depend on the latest version of syntect? |
Nevermind, the update was already done in #671. I rebased your branch onto |
I tested this on my (small) collection of files and couldn't see any obvious regression. I think we should give this a try. |
@benwaffle @keith-hall Thank you! |
This has been released in bat 0.13. |
Fixes #499
Fixes #443