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

Remove unnecessary parenthesis in ON clause of MERGE statement #1021

Merged
merged 1 commit into from
Aug 9, 2020

Conversation

beikov
Copy link
Contributor

@beikov beikov commented Jul 23, 2020

No description provided.

Copy link
Member

@wumpz wumpz left a comment

Choose a reason for hiding this comment

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

Does the Expression production in every case support parenthesis around it? Even for relations?

@beikov
Copy link
Contributor Author

beikov commented Jul 26, 2020

Not sure I understand the question. AFAICS an expression can have parenthesis around for "grouping" purposes anyway, so removing the explicit parenthesis should be fine.

@wumpz
Copy link
Member

wumpz commented Jul 26, 2020

That's what I ment. At the moment I am not able to merge. Give me some time.

@wumpz wumpz merged commit 3d507b4 into JSQLParser:master Aug 9, 2020
wumpz added a commit that referenced this pull request Aug 9, 2020
wumpz added a commit that referenced this pull request Aug 9, 2020
@wumpz
Copy link
Member

wumpz commented Aug 9, 2020

Did you check your changes? I had many failing tests after merging it. So for now I reverted it.

@beikov
Copy link
Contributor Author

beikov commented Aug 10, 2020

I did not check the tests. I thought that this was an easy fix because optional parenthesis are already defined by the expression rule, but if this is more involved, I guess it's not worth pursuing this further, especially since Oracle mandates the same syntax.
Would be great to have feedback on the PR about the test runs though. Have you thought about setting up Github Actions or Travis CI for this?

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