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

Regression on LineEndConcatenation #1054

Closed
agrimm opened this issue Apr 30, 2014 · 9 comments
Closed

Regression on LineEndConcatenation #1054

agrimm opened this issue Apr 30, 2014 · 9 comments
Labels

Comments

@agrimm
Copy link
Contributor

agrimm commented Apr 30, 2014

Recent changes in LineEndConcatenation means that

def foo
  'Hello ' +
  # OPTIMIZE: Turn this into a constant.
  'world'
end

is regarded as incorrect.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 30, 2014

Ops, seems we didn't have a test case for that scenario. I'll have that fixed.

@bbatsov bbatsov self-assigned this Apr 30, 2014
@bbatsov bbatsov added the bug label Apr 30, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented May 10, 2014

/cc @barunio

@bbatsov bbatsov assigned jonas054 and unassigned bbatsov and jonas054 Jul 30, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2014

@jonas054 @yujinakayama I played a bit with this, but didn't manage to solve it in an easy manner (without gutting the cop). I'd appreciate it if you take a look - perhaps you'll find a solution I'm missing.

@jonas054
Copy link
Collaborator

How about something like

          comment_in_between = processed_source.comments.find do |c|
            c.loc.line > receiver.loc.line && c.loc.line < first_arg.loc.line
          end
          return if comment_in_between

towards the end of offending_node?. It's a linear search through all comments of a file, but placed late in the method that might be ok.

Seems obvious. Maybe too obvious. I might be missing something. 😄

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2014

@jonas054 Think I did something similar initially, but it messed up the reporting of the multiple offenses. As I played with this a few weeks back my memory's a bit fuzzy on the subject.

@yujinakayama
Copy link
Collaborator

Just a random thought, maybe this can be handled by inspecting tokens rather than AST? Since we are interested only in string literals, there's enough information in tokens. Though this requires gutting the cop.

[6] pry(main)> RuboCop::ProcessedSource.new(%('foo' +\n'bar')).tokens
=> [#<RuboCop::Token:0x007f9087818980
  @pos=#<Parser::Source::Range (string) 0...5>,
  @text="foo",
  @type=:tSTRING>,
 #<RuboCop::Token:0x007f90878188e0
  @pos=#<Parser::Source::Range (string) 6...7>,
  @text="+",
  @type=:tPLUS>,
 #<RuboCop::Token:0x007f9087818840
  @pos=#<Parser::Source::Range (string) 8...13>,
  @text="bar",
  @type=:tSTRING>]
[7] pry(main)> RuboCop::ProcessedSource.new(%('foo' \\\n'bar')).tokens
=> [#<RuboCop::Token:0x007f90824e6e60
  @pos=#<Parser::Source::Range (string) 0...5>,
  @text="foo",
  @type=:tSTRING>,
 #<RuboCop::Token:0x007f90824e6dc0
  @pos=#<Parser::Source::Range (string) 8...13>,
  @text="bar",
  @type=:tSTRING>]
[8] pry(main)> RuboCop::ProcessedSource.new(%('foo' +\n# some comment\n'bar')).tokens
=> [#<RuboCop::Token:0x007f9087663a18
  @pos=#<Parser::Source::Range (string) 0...5>,
  @text="foo",
  @type=:tSTRING>,
 #<RuboCop::Token:0x007f9087663978
  @pos=#<Parser::Source::Range (string) 6...7>,
  @text="+",
  @type=:tPLUS>,
 #<RuboCop::Token:0x007f90876638d8
  @pos=#<Parser::Source::Range (string) 8...22>,
  @text="# some comment",
  @type=:tCOMMENT>,
 #<RuboCop::Token:0x007f9087663838
  @pos=#<Parser::Source::Range (string) 23...28>,
  @text="bar",
  @type=:tSTRING>]

@bbatsov
Copy link
Collaborator

bbatsov commented Jul 30, 2014

I like @yujinakayama's idea (it's something I considered as well) and I think it'd yield performance improvements over the existing solution.

@yujinakayama
Copy link
Collaborator

I tried to rework the cop with token information and it looked perfect, but it turned out there's a case that cannot be handled with tokens.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 25, 2014

Can't we just do a check if the string is followed by * and % - I think they are the only problematic operators.

mvz added a commit to mvz/alexandria-book-collection-manager that referenced this issue Sep 24, 2014
This error was caused by the previous LineEndConcationation
autocorrection.

See possibly rubocop/rubocop#1054
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

4 participants