-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
Only apply SquishSQLHeredoc when the SQL has no comments #929
Conversation
12ca672
to
438b808
Compare
@@ -48,6 +48,7 @@ class SquishedSQLHeredocs < Base | |||
SQL = 'SQL' | |||
SQUISH = '.squish' | |||
MSG = 'Use `%<expect>s` instead of `%<current>s`.' | |||
SQL_IDENTIFIER_MARKERS = [/".+?"/.freeze, /'.+?'/.freeze, /\[.+?\]/.freeze].freeze |
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.
It could be replaced with a regular expression using gsub
as in the comments below.
SQL_IDENTIFIER_MARKERS = [/".+?"/.freeze, /'.+?'/.freeze, /\[.+?\]/.freeze].freeze | |
SQL_IDENTIFIER_MARKERS = /(".+?")|('.+?')|(\[.+?\])/.freeze |
def singleline_comments_present?(node) | ||
text = node.children&.map { |c| c.is_a?(String) ? c : c.value }&.join('\n') | ||
|
||
return false if text.nil? | ||
|
||
SQL_IDENTIFIER_MARKERS.each { |m| text = text.sub(m, '') } | ||
|
||
text.include?('--') | ||
end |
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.
def singleline_comments_present?(node) | |
text = node.children&.map { |c| c.is_a?(String) ? c : c.value }&.join('\n') | |
return false if text.nil? | |
SQL_IDENTIFIER_MARKERS.each { |m| text = text.sub(m, '') } | |
text.include?('--') | |
end | |
def singleline_comments_present?(node) | |
sql = node.children.map { |c| c.is_a?(String) ? c : c.source }.join('\n') | |
sql.gsub(SQL_IDENTIFIER_MARKERS, '').include?('--') | |
end |
I left several comments. Can you update and add a changelog entry?
|
Thank you, have addressed the comments |
This PR can be merged after CI is passed. Can you fix and squash your commits into one? |
As in the squash-and-merge option on the PR or as in squashing and repushing? |
Yeah, please do |
ded2409
to
da25385
Compare
All squashed. Out of curiosity, is there any advantage to this approach instead of squash-and-merge? |
It looks like there are commits that haven't been squashed yet :-)
"Squash and merge" changes the commit hash. This will cause broken links, etc. For example, it will take more effort to acquire released tag from PR commits merged into mater. And many more. So, I prefer "Create a merge commit" as a long time maintainer. Of course it depends on maintainers, and I don't want to deny other opinions. |
Removes all strings and identifiers from the SQL and then checks it for single-comment markers, and does not apply the cop if they are present
97beeff
to
325a4ca
Compare
Ugh sorry I committed the suggestion from GitHub and didn't think about squashing that. Done now
Interesting, thanks! |
Thank you, too! |
Removes all strings and identifiers from the SQL and then checks it for single-comment markers, and does not apply the cop if they are present