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

Only apply SquishSQLHeredoc when the SQL has no comments #929

Merged
merged 1 commit into from Jan 27, 2023

Conversation

john-h-k
Copy link
Contributor

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

@john-h-k john-h-k force-pushed the fix/sql-squish-comments branch 2 times, most recently from 12ca672 to 438b808 Compare January 26, 2023 18:02
@@ -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
Copy link
Member

@koic koic Jan 27, 2023

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.

Suggested change
SQL_IDENTIFIER_MARKERS = [/".+?"/.freeze, /'.+?'/.freeze, /\[.+?\]/.freeze].freeze
SQL_IDENTIFIER_MARKERS = /(".+?")|('.+?')|(\[.+?\])/.freeze

Comment on lines 75 to 79
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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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

@koic
Copy link
Member

koic commented Jan 27, 2023

I left several comments. Can you update and add a changelog entry?

Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@john-h-k
Copy link
Contributor Author

Thank you, have addressed the comments

@koic
Copy link
Member

koic commented Jan 27, 2023

This PR can be merged after CI is passed. Can you fix and squash your commits into one?

@john-h-k
Copy link
Contributor Author

As in the squash-and-merge option on the PR or as in squashing and repushing?

@koic
Copy link
Member

koic commented Jan 27, 2023

squashing and repushing?

Yeah, please do git rebase -i, squash, and force push to the branch.

@john-h-k john-h-k force-pushed the fix/sql-squish-comments branch 3 times, most recently from ded2409 to da25385 Compare January 27, 2023 11:06
@john-h-k
Copy link
Contributor Author

All squashed. Out of curiosity, is there any advantage to this approach instead of squash-and-merge?

@koic
Copy link
Member

koic commented Jan 27, 2023

All squashed.

It looks like there are commits that haven't been squashed yet :-)

Out of curiosity, is there any advantage to this approach instead of squash-and-merge?

"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.
#747 (comment)

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
@john-h-k
Copy link
Contributor Author

It looks like there are commits that haven't been squashed yet :-)

Ugh sorry I committed the suggestion from GitHub and didn't think about squashing that. Done now

"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.
#747 (comment)

Interesting, thanks!

@koic koic merged commit 8f10b28 into rubocop:master Jan 27, 2023
@koic
Copy link
Member

koic commented Jan 27, 2023

Thank you, too!

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

2 participants