Skip to content

Conversation

@xiedeyantu
Copy link
Member

@xiedeyantu
Copy link
Member Author

Conflicts in #4683 has been resolved.

@sonarqubecloud
Copy link

@xiedeyantu
Copy link
Member Author

I'd like to wait and see if @julianhyde has any other comments.

@xiedeyantu xiedeyantu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Dec 14, 2025
@xiedeyantu
Copy link
Member Author

If there are no further comments within 48 hours, I will consider merging this PR.

@xiedeyantu xiedeyantu added discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR and removed LGTM-will-merge-soon Overall PR looks OK. Only minor things left. labels Dec 14, 2025
@xiedeyantu
Copy link
Member Author

The discussion is still ongoing on Jira, and we may need to wait for @julianhyde’s input.

@xiedeyantu
Copy link
Member Author

This PR hasn’t received a response from Julian yet. I’m not sure if this will add to the parser’s burden, so should we close this PR?

@julianhyde
Copy link
Contributor

I hate reviewing PRs, so I'm not going to +1 this, but you don't need a +1 from me. If the specification is clear, and the code implements the specification, go ahead and merge.

@xiedeyantu
Copy link
Member Author

I hate reviewing PRs, so I'm not going to +1 this, but you don't need a +1 from me. If the specification is clear, and the code implements the specification, go ahead and merge.

Thank you for your reply. I will add the test cases mentioned in Jira to this PR and continue to complete it.

@xiedeyantu xiedeyantu removed the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Jan 14, 2026
@xiedeyantu
Copy link
Member Author

I added some tests to the quidem in Babel (from a discussion on Jira), and the conflicts in the main branch of the rebase were resolved. There were no other changes. @mihaibudiu If you have time, could you please check if it's ready?

@sonarqubecloud
Copy link

SqlIdentifier id;
}
{
<EXCLUDE> <LPAREN> { s = span(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

this could potentially be configurable, but I guess that people can adapt the parser and choose what they want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we'll keep it simple for now, supporting both, and keeping the configuration parameters as minimal as possible.

@xiedeyantu xiedeyantu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants