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

Style/TrailingWhitespace marks offenses when whitespace is in a string #4517

Closed
drbrain opened this issue Jun 14, 2017 · 9 comments
Closed
Labels

Comments

@drbrain
Copy link

drbrain commented Jun 14, 2017

Expected behavior

Writing a multiline string with trailing whitespace for exact formatting must not cause a Style/TrailingWhitespace offense because rubocop cannot know the intended use of a string.

Actual behavior

Rubocop marks strings with trailing whitespace as a Style/TrailingWhitespace offense

Steps to reproduce the problem

Write t.rb:

STRING = <<-STRING
This line has a trailing space: 

This line has a trailing tab:	

Nothing in this file should cause a Style/TrailingWhitespace offense.
STRING

run rubocop:

$ rubocop t.rb
Inspecting 1 file
C

Offenses:

t.rb:1:10: C: Freeze mutable objects assigned to constants.
STRING = <<-STRING
         ^^^^^^^^^
t.rb:2:1: C: Use 2 spaces for indentation in a heredoc by using some library(e.g. ActiveSupport's String#strip_heredoc).
This line has a trailing space:  ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
t.rb:2:32: C: Trailing whitespace detected.
This line has a trailing space: 
                               ^
t.rb:4:30: C: Trailing whitespace detected.
This line has a trailing tab:	
                             ^

1 file inspected, 4 offenses detected

I do not expect the cops on lines 2 or 4 to appear.

RuboCop version

Include the output of rubocop -V. Here's an example:

$ rubocop -V
0.49.1 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-darwin16)
@jonas054 jonas054 added the bug label Jun 19, 2017
@smakagon
Copy link
Contributor

smakagon commented Jul 4, 2017

Guys, I've looked into implementation of this cop. Wanted to fix this bug, but I have couple questions.

Current implementation looks like this:

        def investigate(processed_source)
          processed_source.lines.each_with_index do |line, index|
            next unless line.end_with?(' ', "\t")
            # couple more lines
          end
        end

So it goes through each line of code. Each line of code is a string.

To fix this bug I need to work with line of code as with node, right?
I found mixin called Heredoc which provides method on_heredoc. I could use that one.

Do you have any suggestions on proper implementation for this case?

Btw, for this cop there is an example that says: it registers an offense for trailing whitespace in a heredoc string , so it was expected behavior for author of the cop. :)

@Drenmi
Copy link
Collaborator

Drenmi commented Jul 5, 2017

@smakagon: The high level strategy I would use might be:

  1. Store all the heredoc line ranges in the file in a collection.
  2. When checking all the lines for trailing spaces, also check that it isn't in the collection from 1.

it was expected behavior for author of the cop

Yes. The suggested behaviour needs a configuration option. 🙂

@smakagon
Copy link
Contributor

smakagon commented Jul 5, 2017

@Drenmi thanks for the answer.

To store all heredoc line ranges, I need to figure out that it's a heredoc.
It's not possible with current way of processing with: investigate(processed_source).

I was struggling with that because from one side I need to work with code as with an array of strings, but from the other side I want to know if it's a heredoc.

@jonas054
Copy link
Collaborator

jonas054 commented Jul 5, 2017

The investigate method is called before the on_ methods. If it was the other way around we'd be laughing. Now it's a bit tricky to figure out how we can first store all the line numbers where there are heredocs, and then check all lines. Or did you have a solution in mind for this @Drenmi?

@Drenmi
Copy link
Collaborator

Drenmi commented Jul 13, 2017

@jonas054: Hm. That definitely makes it less straightforward than I envisioned. 🤔

@jonas054
Copy link
Collaborator

@Drenmi I played around a little and tried the following:

diff --git a/lib/rubocop/cop/layout/trailing_whitespace.rb b/lib/rubocop/cop/layout/trailing_whitespace.rb
index a662b5b..5d70a74 100644
--- a/lib/rubocop/cop/layout/trailing_whitespace.rb
+++ b/lib/rubocop/cop/layout/trailing_whitespace.rb
@@ -5,6 +5,7 @@ module RuboCop
     module Layout
       # This cop looks for trailing whitespace in the source code.
       class TrailingWhitespace < Cop
+        include Heredoc
         MSG = 'Trailing whitespace detected.'.freeze
 
         def investigate(processed_source)
@@ -19,6 +20,11 @@ module RuboCop
           end
         end
 
+        def on_heredoc(node)
+          heredoc_body = node.loc.heredoc_body
+          offenses.reject! { |o| heredoc_body.overlaps?(o.location) }
+        end
+
         def autocorrect(range)
           ->(corrector) { corrector.remove(range) }
         end

This is perhaps a bit ugly. We haven't used offenses.reject! in any cops before, removing already added offenses. But it does get the job done.

@reitzig
Copy link

reitzig commented Feb 21, 2018

RubuCop still seems to want to treat heredoc strings as code, at least in some cops:

puts <<~MSG
      \nSome message: #{mystuff}
        This is bad!
  MSG

2018-02-21_01
2018-02-21_02

This is in RubyMine 2017.3.2 with RubuCop 0.52.1.

@jonas054
Copy link
Collaborator

@reitzig Squiggly heredocs (~MSG) were introduced in Ruby 2.3. That's why the parser gem has problems with the syntax.

Add this to your configuration:

AllCops:
  TargetRubyVersion: 2.3

@reitzig
Copy link

reitzig commented Feb 22, 2018

That makes sense. I'll have to see how to configure this for the "built-in" RuboCop in RubyMine. Thanks!

For reference: there are issues on the RubyMine bugtracker about this, e.g. this.

Amendment: Adding a .rubocop.yml to the project root was enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants