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

Replace the two booleans from 'SET ROLE' with a single enum ContextModifier #664

Merged
merged 1 commit into from Oct 10, 2022
Merged

Replace the two booleans from 'SET ROLE' with a single enum ContextModifier #664

merged 1 commit into from Oct 10, 2022

Conversation

AugustoFKL
Copy link
Contributor

@AugustoFKL AugustoFKL commented Oct 9, 2022

This way, we don't rely on the parser to have valid structures for that statement, as the structure itself is valid.

Postgresql doesn't allow both to be present (1), so there's no meaning in having a structure that allows both. Also, ANSI (2) and a lot of other dialects support that command, so having the comment PostgreSQL specific doesn't make sense.

@alamb This is ready for review. Also, do you know why the coverage is so weird? Like, a lot of enum matches is not covered, even though their interior is

[1] : https://www.postgresql.org/docs/current/sql-set-role.html
[2] : https://jakewheat.github.io/sql-overview/sql-2016-foundation-grammar.html#set-role-statement

This way, we don't rely on the parser to have valid structures for that
statement, as the structure itself is valid.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3215185150

  • 24 of 28 (85.71%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.005%) to 85.794%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 8 12 66.67%
Totals Coverage Status
Change from base Build 3209998484: 0.005%
Covered Lines: 10484
Relevant Lines: 12220

💛 - Coveralls

Copy link
Collaborator

@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.

Makes sense to me -- thank you @AugustoFKL

@alamb alamb changed the title Replacing the two booleans from 'SET ROLE' to a single enum. Replacing the two booleans from 'SET ROLE' with a single enum ContextModifier Oct 10, 2022
@alamb alamb changed the title Replacing the two booleans from 'SET ROLE' with a single enum ContextModifier Replace the two booleans from 'SET ROLE' with a single enum ContextModifier Oct 10, 2022
@alamb alamb merged commit 7776726 into sqlparser-rs:main Oct 10, 2022
@AugustoFKL AugustoFKL deleted the improving-set-role-possible-scenarios branch November 5, 2022 23:11
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.

None yet

3 participants