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

Add new Rails/SquishedSQLHeredocs cop #291

Merged
merged 1 commit into from
Aug 20, 2020
Merged

Add new Rails/SquishedSQLHeredocs cop #291

merged 1 commit into from
Aug 20, 2020

Conversation

mobilutz
Copy link
Contributor

@mobilutz mobilutz commented Jul 9, 2020

This cops check <<-SQL SQL-heredocs if they use .squish or not.

Base is this styleguide: Squished Heredocs


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

Copy link
Contributor

@eugeneius eugeneius left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

config/default.yml Outdated Show resolved Hide resolved
config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_heredocs.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rails/squished_heredocs_spec.rb Outdated Show resolved Hide resolved
spec/rubocop/cop/rails/squished_heredocs_spec.rb Outdated Show resolved Hide resolved
@mobilutz mobilutz marked this pull request as draft July 11, 2020 21:59
@mobilutz mobilutz changed the title Add new Rails/SquishedHeredocs cop Add new Rails/SquishedSQLHeredocs cop Jul 13, 2020
@mobilutz mobilutz marked this pull request as ready for review July 15, 2020 21:04
@eugeneius
Copy link
Contributor

Sorry @mobilutz, I just realised that my earlier suggestion about single-line heredocs was wrong. Even when a heredoc is only a single line, it still always has a trailing newline character:

$ ruby -e "puts <<~SQL.inspect
SELECT * FROM posts
SQL"
"SELECT * FROM posts\n"

Since the point of the style guide entry is to avoid logging newline characters, I think we should actually enforce squish in all cases, as you had originally. 😖

@mobilutz
Copy link
Contributor Author

Since the point of the style guide entry is to avoid logging newline characters, I think we should actually enforce squish in all cases, as you had originally. 😖

Thanks for testing it out again, I totally did not do that.
I will revert the single line check so that squish will be used on all heredocs.

@mobilutz
Copy link
Contributor Author

mobilutz commented Aug 5, 2020

@eugeneius Any chance you could review this again?
Thanks

Copy link
Contributor

@eugeneius eugeneius left a comment

Choose a reason for hiding this comment

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

Here's a few changes that make the inline case work correctly:

diff --git a/spec/rubocop/cop/rails/squished_sql_heredocs_spec.rb b/spec/rubocop/cop/rails/squished_sql_heredocs_spec.rb
index 340d317a3..a3b132c00 100644
--- a/spec/rubocop/cop/rails/squished_sql_heredocs_spec.rb
+++ b/spec/rubocop/cop/rails/squished_sql_heredocs_spec.rb
@@ -58,8 +58,16 @@ RSpec.describe RuboCop::Cop::Rails::SquishedSQLHeredocs, :config do

   context 'with heredocs as method parameters' do
     it 'does not register an offense' do
-      expect_no_offenses(<<-RUBY)
+      expect_offense(<<~RUBY)
         execute(<<~SQL, "Post Load")
+                ^^^^^^ Use `<<~SQL.squish` instead of `<<~SQL`.
+          SELECT * FROM posts
+            WHERE post_id = 1
+        SQL
+      RUBY
+
+      expect_correction(<<~RUBY)
+        execute(<<~SQL.squish, "Post Load")
           SELECT * FROM posts
             WHERE post_id = 1
         SQL

lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
@mobilutz
Copy link
Contributor Author

mobilutz commented Aug 9, 2020

@eugeneius Thank you very much for the great input! Now even method-parameter Heredocs are working!!!

I rebased origin/master and added hopefully all documentation etc.
Is there anything else I can do to make this PR mergeable?

Copy link
Contributor

@eugeneius eugeneius left a comment

Choose a reason for hiding this comment

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

This is looking good to me! I added a few more suggestions.

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rails/squished_sql_heredocs.rb Outdated Show resolved Hide resolved
@koic
Copy link
Member

koic commented Aug 20, 2020

@mobilutz Can you update this PR?

@mobilutz
Copy link
Contributor Author

@mobilutz Can you update this PR?

Thanks for the reminder.

@eugeneius Thanks for your great reviews!

Copy link
Contributor

@eugeneius eugeneius left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for sticking with this @mobilutz!

@koic koic merged commit 9e2ff2c into rubocop:master Aug 20, 2020
@koic
Copy link
Member

koic commented Aug 20, 2020

Thank you!

@mobilutz mobilutz deleted the ll-squished-heredocs branch August 21, 2020 10:30
@mobilutz
Copy link
Contributor Author

mobilutz commented Aug 21, 2020

@koic It seems, that the changelog entry is not in the correct place 😦
v2.7.1...master#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR43

PS: I create a new PR to fix this: #333

@koic
Copy link
Member

koic commented Aug 21, 2020

@mobilutz Good catch! I updated the changelog. Thank you!
ef26793

lxxxvi added a commit to lxxxvi/marconi that referenced this pull request Sep 14, 2020
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.

3 participants