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

Language Server - Parse all - Transition from Exceptions to Diagnostics #339

Merged
merged 3 commits into from
Dec 15, 2021

Conversation

fredg1
Copy link
Contributor

@fredg1 fredg1 commented Dec 14, 2021

No description provided.

@fredg1 fredg1 requested a review from a team as a code owner December 14, 2021 00:09
@gausie
Copy link
Contributor

gausie commented Dec 14, 2021

If this allows multiple errors to be collected per script (rather than throwing out at the first one), I'd like to see some tests that describe that behaviour

@fredg1
Copy link
Contributor Author

fredg1 commented Dec 14, 2021

If this allows multiple errors to be collected per script (rather than throwing out at the first one), I'd like to see some tests that describe that behaviour

Would it be okay if it was only a single test?
Currently, the main issue with these errors is that they aren't filtered at all (which is the next step).
This means that if we submit a script with syntax errors, the diagnostics get clogged with a couple of unwanted errors.

So, I'd only add a test with simple errors, like a break outside of a loop, unknown import, invalid aggregate key, etc.

@gausie
Copy link
Contributor

gausie commented Dec 14, 2021

If this allows multiple errors to be collected per script (rather than throwing out at the first one), I'd like to see some tests that describe that behaviour

Would it be okay if it was only a single test? Currently, the main issue with these errors is that they aren't filtered at all (which is the next step). This means that if we submit a script with syntax errors, the diagnostics get clogged with a couple of unwanted errors.

So, I'd only add a test with simple errors, like a break outside of a loop, unknown import, invalid aggregate key, etc.

I'm just asking you to add testing to this PR that fully covers behaviour you are adding in this PR

Copy link
Contributor

@gausie gausie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!

@gausie gausie merged commit dbaab2e into kolmafia:main Dec 15, 2021
@fredg1 fredg1 deleted the LS-parse-all-diagnostics branch December 16, 2021 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants