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

extend syntax error information #879

Merged
merged 11 commits into from
Mar 30, 2024

Conversation

Simulant87
Copy link
Contributor

The syntax error contains only the index of the not parsable character. It might be helpful to indclude more information like the current character and why it is not allowed at this place.

@stleary
Copy link
Owner

stleary commented Mar 23, 2024

@Simulant87 Can you include unit tests for the new behavior?

@Simulant87
Copy link
Contributor Author

Simulant87 commented Mar 23, 2024

@stleary Sure, I added tests for the new exception messages.

I am not sure, if you like my latest change, explainging the numbers in the syntax error with (zero based) "index" and (1 based) "number in line". I think this makes it easier to find the error when counting for the wrong character in the input string. But it requires a lot of tests to be updated.

If you don't like it, I can also revert it again. But I think it makes the error messages easier to read.

@stleary
Copy link
Owner

stleary commented Mar 28, 2024

What problem does this code solve?
Some tokener parsing error messages are not sufficiently descriptive

Does the code still compile with Java6?
Yes

Risks
Low. It's possible that some users' unit tests may fail due to this change. I think the value of the new messages outweighs this risk.

Changes to the API?
No, just changes to some parsing error messages.

Will this require a new release?
No

Should the documentation be updated?
No

Does it break the unit tests?
No, there was no previous coverage for these errors. New tests were added.

Was any code refactored in this commit?
No

Review status
APPROVED

Starting 3-day comment window.

@stleary stleary merged commit 87406e4 into stleary:master Mar 30, 2024
7 checks passed
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