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: Don't report an error about testcases in the LSP #5086

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Aug 12, 2022

We should only need to care about testcases when actually serializing to flatbuffer, so just make that error instead of panicking and remove the pre-check.

Fixes influxdata/flux-lsp#403 (again? the LSP seems to run semantic checks now which is why it was reintroced/is still reporting this error)

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

@Marwes Marwes requested a review from a team as a code owner August 12, 2022 11:01
@Marwes Marwes requested review from jsternberg and removed request for a team August 12, 2022 11:01
Markus Westerlind added 3 commits September 20, 2022 14:41
We should only need to care about this when actually serializing to flatbuffer, so just make that error instead of panicking
Markus Westerlind added 2 commits September 20, 2022 14:44
We didn't hit this code path before as testcases never passed through it but now that testcases can
(with the LSP) we need to handle it correctly.
@Marwes
Copy link
Contributor Author

Marwes commented Sep 20, 2022

ping @jsternberg

@Marwes Marwes merged commit 9738cff into master Sep 20, 2022
@Marwes Marwes deleted the allow_testcase_in_lsp branch September 20, 2022 14:52
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.

TestCase is not supported in semantic analysis
2 participants