bugfix for HighlightState with non-empty initial stack #263
Conversation
There was a problem hiding this comment.
Thanks for looking into this. I like that your fix is simple, and you included a new unit test. 👍 However, (sorry!), I don't think the test proves what we want it to at the moment... I will explain. The test is checking the highlighting of a comment using the Spacegray/Base 16 Ocean Dark color scheme. The scope selector it uses for matching comments is comment, punctuation.definition.comment. This is a compound selector, checking if either of those two scopes match. But the syntect highlighter state only caches selectors which target single scopes. So I think it doesn't prove that those are being cached and treated correctly atm, but I could be wrong. That said, this is still better than no test and definitely better than panicing, so even merging this as-is would be an improvement. :)
|
Thanks for reviewing @keith-hall ❤️ My goal with this test was to create a scenario that would break prior to the change, and produce the expected result after. I totally get (the idea of) the test not actually replicating the most-useful way of using the feature, and not covering all the execution paths, but also not sure how I would test that(?) At any rate as you said it's an improvement to crashing, so perhaps you could merge it and we would continue the cache testing discussion independently? |
|
I am amenable to that, but although I have the technical permissions to merge a PR, this is @trishume's repo and ultimately the decision should be his :) |
I believe this fixes issue #261, though I don't feel confident enough with the code base to assure this is the exact intended behaviour, so definitely needs a review from someone who knows their way around this repo :)