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

Guard Values against null/empty values #1965

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

pingpingy1
Copy link
Contributor

This PR intends to implement suitable guards for the constructors of the classes DoubleValue, LongValue, and TimeValue.
Both null and empty strings provided to their constructors fail, but they provide very different error messages (NullPointerException and StringIndexOutOfBoundsException),
which is neither sensible nor helpful in debugging.
This PR adds a guard to throw IllegalArgumentException for both cases in order to improve coherency and usefulness of the error messages.

The classes modified by this commit are `DoubleValue`, `LongValue`, and
`TimeValue`. Both `null` and empty strings provided to their
constructors fail, but they provide very different error messages
(NullPointerException and StringIndexOutOfBoundsException), which is
neither sensible nor helpful in debugging.
This commit adds a guard to throw `IllegalArgumentException` for both
cases in order to improve coherency and usefulness of the error
messages.
@manticore-projects
Copy link
Contributor

Looks good.
Please fix the formatting by running:

./gradlew :spotlessApply

and then I would merge.

@pingpingy1
Copy link
Contributor Author

@manticore-projects Thank you for the swift response.
I believe that should have fixed it; PTAL.

@manticore-projects manticore-projects merged commit b00322e into JSQLParser:master Feb 14, 2024
3 checks passed
@manticore-projects
Copy link
Contributor

Thank you for your contribution, much appreciated!

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