Skip to content
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

Fix analyzer not being called when getHighlightParsingError is off #396

Merged
merged 1 commit into from
May 14, 2022
Merged

Fix analyzer not being called when getHighlightParsingError is off #396

merged 1 commit into from
May 14, 2022

Conversation

shabbyrobe
Copy link
Contributor

My apologies, looks like I let a regression slip through. I inadvertently allowed the call to the analyzer to be gated behind the getHighlightParsingError function, 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 config is global. @skovhus, are you comfortable with me replacing accesses to the global config functions with an interface type passed in as a dependency to BashServer? 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 undefined can come out of uriToTreeSitterTrees, and we were trying to access rootNodes against a document even if it wasn't found.

@codecov
Copy link

codecov bot commented May 14, 2022

Codecov Report

Merging #396 (14b2528) into master (95dabb4) will not change coverage.
The diff coverage is 0.00%.

@@           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           
Impacted Files Coverage Δ
server/src/analyser.ts 83.23% <0.00%> (ø)
server/src/server.ts 57.44% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 213a517...14b2528. Read the comment docs.

Copy link
Collaborator

@skovhus skovhus left a 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.

@skovhus skovhus merged commit 360b4f4 into bash-lsp:master May 14, 2022
@skovhus
Copy link
Collaborator

skovhus commented May 14, 2022

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.

@shabbyrobe
Copy link
Contributor Author

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.

@shabbyrobe
Copy link
Contributor Author

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?

@skovhus
Copy link
Collaborator

skovhus commented May 14, 2022

I've got an idea, is it OK if I submit a draft PR?

anytime!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants