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

ForceEqualSignAlignment shouldn't use method definition in alignment #2722

Closed
Dbz opened this issue Jan 26, 2016 · 8 comments
Closed

ForceEqualSignAlignment shouldn't use method definition in alignment #2722

Dbz opened this issue Jan 26, 2016 · 8 comments

Comments

@Dbz
Copy link

Dbz commented Jan 26, 2016

$ rubocop -V
0.36.0 (using Parser 2.3.0.1, running on ruby 2.3.0 x86_64-darwin14)
# .rubocop.yml
Style/ExtraSpacing:
  ForceEqualSignAlignment: true

Running rubocop -a turns the following dfs method:

def dfs(node = @root)
  nodes = []
end

into this dfs method:

def dfs(node = @root)
  nodes      = []
end

Rubocop shouldn't take the method definition into consideration when forcing equal sign alignment. I can fix this by adding or modifying the .select statement within the def investigate(processed_source) method in extra_spacing.rb.

@Dbz Dbz changed the title ForceEqualSignAlignment shouldn't check method definition ForceEqualSignAlignment shouldn't use method definition in alignment Jan 27, 2016
@jfelchner
Copy link
Contributor

@alexdowad I think that one of the keys here is that, for reference of proper alignment, the cop needs to go up line by line until it gets to either:

  • a line of lesser indentation level as the current line
  • a line of the same indentation level as the current line

In that way, both of these (currently broken) cases will be caught:

Previous Line of Lesser Indentation Contains Token

class Foo
  def here_is_a_long_method_name(options = {})
    self.bar = 'cool'
    self.bazinga = 'sweet'
  end
end

Should be corrected to:

class Foo
  def here_is_a_long_method_name(options = {})
    self.bar     = 'cool'
    self.bazinga = 'sweet'
  end
end

Instead of:

class Foo
  def here_is_a_long_method_name(options = {})
    self.bar                             = 'cool'
    self.bazinga                         = 'sweet'
  end
end

Previous Line of Greater Indentation Does Not Contain Token

class Foo
  def bar(options = {})
    self.bazinga = {
                     one: '1',
                     two: '2',
                   }
    self.bar   = 'cool'
  end
end

Should be corrected to:

class Foo
  def bar(options = {})
    self.bazinga = {
                     one: '1',
                     two: '2',
                   }
    self.bar     = 'cool'
  end
end

Instead of:

class Foo
  def bar(options = {})
    self.bazinga = {
                     one: '1',
                     two: '2',
                   }
    self.bar = 'cool'
  end
end

@alexdowad
Copy link
Contributor

@jfelchner Good points. But I wonder if the suggestion from @Dbz should also be heeded.

def method1(arg1, arg2 = :default); end
def method2(arg3 = :default); end

It would look pretty strange to align the equals signs in those arglists, don't you think?

@jfelchner
Copy link
Contributor

@alexdowad in the rare cases where I have multiple single line methods, I dunno, I might actually like them aligned 😀 but honestly I could go either way with whatever you decide. 👍

You've been killing it lately man. Thank you so much for your work.

@Dbz
Copy link
Author

Dbz commented Feb 5, 2016

I agree with Alex that it might seem awkward to align the two methods- especially because you would be aligning the second parameter from method1 with the first parameter from method2.

I also think there might be a bug where calling rubocop -a fixes a lot of the equal sign alignment, but still throws the error of not being aligned.

I'd be happy to pair with you @alexdowad on working out a solution to fix these cops if you're up for it. It would give me better insight to the codebase.

@alexdowad
Copy link
Contributor

I'd be happy to pair with you @alexdowad on working out a solution to fix these cops if you're up for it. It would give me better insight to the codebase.

Your suggested fix using .select will work fine. (It won't cover all the scenarios presented by @jfelchner, of course.) Would you like help on how to code the body of the select?

@Dbz
Copy link
Author

Dbz commented Feb 6, 2016

Oh, I can definitely do the rather simple select fix. I was talking about your approach by working upwards and tracking the bug where it says the lines are in violation after fixing them even though they aren't. I'll post a code example this weekend of that.

@jfelchner
Copy link
Contributor

@alexdowad would there be any way, while you're in that cop, to make it handle my second use case above? Where the cop looks at the token placement on the previous line of equal indentation?

@jfelchner
Copy link
Contributor

@Dbz I re-read this issue and saw your example for the alignment of those method definitions with fresh eyes. I didn't even notice that the first one had two arguments hahahaha Definitely shouldn't align the equal signs on those. 😉

@bbatsov bbatsov closed this as completed in a0117d5 Jun 6, 2016
Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
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

No branches or pull requests

3 participants