-
Notifications
You must be signed in to change notification settings - Fork 137
Fix analyzer not being called when getHighlightParsingError is off #396
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
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #396 +/- ## ======================================= Coverage 73.96% 73.96% ======================================= Files 19 19 Lines 653 653 Branches 112 112 ======================================= Hits 483 483 Misses 144 144 Partials 26 26
Continue to review full report at Codecov.
|
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.
Thanks for the quick fix! 👏 It would be great to refactor this to enable testing. I have no strong opinions on the best option here.
What I suggest is that we reconsider if we should just get rid of the config option. This really is a workaround as the tree sitter grammar isn’t complete. Any downsides to removing it? A user will not get feedback on why some LSP features doesn’t work. 🤔
To test the behaviour we should be able to mock the analyzer and lint function.
What I suggest is that we reconsider if we should just get rid of the config option.
I have been keeping it turned off actually, it wasn't typically telling me anything shellcheck wasn't doing a better job of telling me. Maybe I need to spend a bit more time with it turned on to see if I really feel this way.
Thanks for the quick fix! 👏 It would be great to refactor this to enable testing. I have no strong opinions on the best option here.
No worries! My own fault, glad I ran into it before it inconvenienced lots of folks. As to the config thing, I've got an idea, is it OK if I submit a draft PR?
I've got an idea, is it OK if I submit a draft PR?
anytime!
My apologies, looks like I let a regression slip through. I inadvertently allowed the call to the analyzer to be gated behind the
getHighlightParsingErrorfunction, so if it was off, document trees were never being analysed and cached, breaking lots of things.It's a bit hard to write a test for at the moment, given
configis global. @skovhus, are you comfortable with me replacing accesses to the global config functions with an interface type passed in as a dependency toBashServer? This will make it trivial to inject different values in a test case. Is there a better way to do this?There was also an issue where TypeScript seems to not notice that
undefinedcan come out ofuriToTreeSitterTrees, and we were trying to accessrootNodesagainst a document even if it wasn't found.