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 Style/IndentAssignment Cop #2410

Merged
merged 1 commit into from
Nov 16, 2015
Merged

Conversation

panthomakos
Copy link
Contributor

This is a prequel to #2361. It is enabled by default and properly fixes the auto-correct output that MultilineAssignmentLayout produces.

When assignments span multiple lines, this cop enforces proper indentation on the right-hand-side of the expression. It only corrects the first line, as the remaining lines can be corrected by other cops such as Style/IndentationConsistency.

# bad
value =
if foo
  'bar'
else
  'biz'
end

# good
value =
  if foo # corrected/accepted
  'bar'
else
  'biz'
end

I have also added a fix to Astrolabe::Node#asgn_method_call?. It used to consider != method calls as assignments - which they are not. There were also no tests for this method so I've added them.

require 'spec_helper'

describe Astrolabe::Node do
context '#asgn_method_call?' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this ought to be describe, not context.

@panthomakos panthomakos force-pushed the indent-assignment branch 2 times, most recently from 0803d23 to 18752c1 Compare November 10, 2015 18:35
@panthomakos
Copy link
Contributor Author

@jonas054 updated to address your comment

@jonas054
Copy link
Collaborator

The code looks good, but there's something wrong. The build fails. Please add spec example(s) that fails the same way that the rubocop self-inspection currently does (if you see what I mean), and fix.

@panthomakos
Copy link
Contributor Author

@jonas054 The issue was some additional comparison operators that ast_node was counting as assignment operators (such as ===). I've adjusted the list and added some additional tests. I've also added a backup test to indent_assignment_spec.rb to ensure that missing operator calls don't cause failures.

@jonas054
Copy link
Collaborator

👍 Looks good but you must rebase on top of the latest master.

@panthomakos
Copy link
Contributor Author

@jonas054 rebased on top of master

@@ -37,8 +37,11 @@ def single_line?
!multiline?
end

COMPARISON_OPERATORS = [:==, :===, :!=, :<=, :>=, :>, :<, :<=>]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be frozen and possibly placed higher up the class def. I don't like mixing constants with method definitions.

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 13, 2015

I don't see a test showing that this won't generate an offense:

ala = if bala
           tralala
         end

@bbatsov
Copy link
Collaborator

bbatsov commented Nov 13, 2015

I have also added a fix to Astrolabe::Node#asgn_method_call?. It used to consider != method calls as assignments - which they are not. There were also no tests for this method so I've added them.

Nice!

@alexdowad
Copy link
Contributor

I have also added a fix to Astrolabe::Node#asgn_method_call?

Thanks for fixing my bugs!

When assignments span multiple lines, this cop enforces proper
indentation on the right-hand-side of the expression. It only corrects
the first line, as the remaining lines can be corrected by other cops
such as `Style/IndentationConsistency`.

    # bad
    value =
    if foo
      'bar'
    else
      'biz'
    end

    # good
    value =
      if foo # corrected/accepted
      'bar'
    else
      'biz'
    end

I have also added a fix to `Astrolabe::Node#asgn_method_call?`. It used
to consider comparison operators such as `!=` and `===` as assignments -
which they are not. There were also no tests for this method so I've
added them.
@panthomakos
Copy link
Contributor Author

@bbatsov addressed your comments and rebased off master.

bbatsov added a commit that referenced this pull request Nov 16, 2015
@bbatsov bbatsov merged commit 9485441 into rubocop:master Nov 16, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 16, 2015

👍

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.

4 participants