Skip to content

Improve error messages with additional colons #1319

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

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

LorrensP-2158466
Copy link
Contributor

This should close #1259.

Added colons at the end of Expected and Column in ParserErrors.
Tests are also updated and they passed.

I do think this creates another "issue":
Currently, a lot of errors read nice without the colon after Expected:
"Expected an expression, found: ... "
and others would not read nice:
"Expected (, found: ..."

But with this change, we would flip these experiences:
"Expected: an expression, found: ..." and "Expected (, found: ..."

I do think we should keep the colons, but change the error messages, this is a bit much more work, but if asked i can do it.

@coveralls
Copy link

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9587049775

Details

  • 109 of 122 (89.34%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.991%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tests/sqlparser_hive.rs 3 4 75.0%
tests/sqlparser_snowflake.rs 14 15 93.33%
tests/sqlparser_postgres.rs 10 12 83.33%
tests/sqlparser_common.rs 68 77 88.31%
Totals Coverage Status
Change from base Build 9565999089: 0.0%
Covered Lines: 26190
Relevant Lines: 29430

💛 - Coveralls

@alamb alamb changed the title Missing colon's in error Improve error messages with additional colons Jun 20, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @LorrensP-2158466 -- this looks good to me

Any concerns @jmhain @iffyio or @lovasoa ?

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @LorrensP-2158466!

@alamb alamb merged commit f16c1af into apache:main Jun 21, 2024
10 checks passed
@alamb
Copy link
Contributor

alamb commented Jun 21, 2024

Thanks everyone!

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.

Add missing colon to sql parser error message after "Column" and "Expected"
5 participants