-
Notifications
You must be signed in to change notification settings - Fork 556
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
fixed #1403, improved comment handling #1404
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1404 +/- ##
==========================================
+ Coverage 79.60% 79.67% +0.06%
==========================================
Files 25 25
Lines 3030 3035 +5
==========================================
+ Hits 2412 2418 +6
+ Misses 618 617 -1
☔ View full report in Codecov by Sentry. |
|
||
def remove_beginning_comments(command): | ||
# Regular expression pattern to match comments | ||
pattern = r"^(/\*.*?\*/|--.*?)(?:\n|$)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that sqlparse.format(command, strip_comments=True)
may do exactly what you want. I didn't try it myself, though, so take it with a grain of salt, but I suggest trying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@j-bennet Thank you for taking time to analyze this. If I remember correctly, we decided to not use the strip_comments flags in sqlparse because it truly throws away the comments and some user wanted comments to show up in postgres error/output logs. So this "dance" in the code is to save the beginning comments, remove them from the sql query, parse the sql query, then re-add the comments to the beginning of the query so that they show up in error/output logs.
Does that make sense or would you like to tackle this a different way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged. Thank you @ERYoung11 . |
Description
Fixed #1403, improved comment handling when the comment is on the first line. Also added some more testing for this and simmilar issues as well as rearranged these tests some.
Checklist
changelog.rst
.AUTHORS
file (or it's already there).pip install pre-commit && pre-commit install
), and ranblack
on my code.