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

Differentiate between empty lines at beginning and end #4812

Closed
LandonSchropp opened this issue Sep 29, 2017 · 11 comments
Closed

Differentiate between empty lines at beginning and end #4812

LandonSchropp opened this issue Sep 29, 2017 · 11 comments

Comments

@LandonSchropp
Copy link

To me, the spacing between the class definition and the first method makes this a little difficult to read.

class MyAwesomeClass < MyAwesomeModule::MyAwesomeParentClass
  def my_awesome_method(awesome_parameter)
    ...
  end

  def another_awesome_method
    ...
  end

  def awesome?
    ...
  end
end

Rubocop provides the Layout/EmptyLInesAroundClassBody style, which allows me to add spaces inside the class.

class MyAwesomeClass < MyAwesomeModule::MyAwesomeParentClass

  def my_awesome_method(awesome_parameter)
    ...
  end

  def another_awesome_method
    ...
  end

  def awesome?
    ...
  end

end

However, that extra space at the bottom feels unnecessary to me. At the top, it helps with the readability of the method name and lets me group those two things together. At the bottom, it's just inserting extra whitespace.

What I really want to do is set the line spacing separately for the top and bottom of the class. My ideal would look like this:

class MyAwesomeClass < MyAwesomeModule::MyAwesomeParentClass

  def my_awesome_method(awesome_parameter)
    ...
  end

  def another_awesome_method
    ...
  end

  def awesome?
    ...
  end
end

I could also see adding these types of options for other Layout/EmptyLine styles as well, such as Layout/EmptyLineAroundBlockBodyor Layout/EmptyLineAroundModuleBody.

@ghost
Copy link

ghost commented Sep 29, 2017

I consider the Rubocop rule and behaviour of EmptyLinesAroundClassBody to be correct and sensible.

But I also agree that, for whatever reason, if someone really really wants to use the above, then why not?

I assume it may be trivial to add, since one could use the existing cop rule, and just not add a newline to the bottom part.

You may probably have to convince bbatsov either way.

PS: I think it may be easier if you just focus on one suggestion in this issue request; the other three could be added at a later time. Sometimes it takes a little while for things to change.

What name would you give the new rule by the way? Coming up with a new name is often difficult; old name is EmptyLInesAroundClassBody (probably misspelled? EmptyLinesAroundClassBody .. but let's ignore the typo anyway)

@LandonSchropp
Copy link
Author

LandonSchropp commented Oct 2, 2017

Thanks for the quick reply! I also tend to use the above style in specs, because it helps the file to be more readable without the end getting too spaced out.

describe "MyAwesomeClass" do

  describe "#my_awesome_method" do

    context "when it's awesome" do

      context "and when the parameter is awesome" do

        it "returns something else that's awesome" do
          ...
        end
      end
    end
  end
end

But I totally understand I might be in the minority in liking this coding style.

As far as naming suggestions, you could split this property into Layout/EmptyLinesBeforeClassBody and Layout/EmptyLinesAfterClassBody, which seems consistent with the other Layout rules. However, that might cause some pain for other people who already have a Layout/EmptyLinesAroundClassBody rule defined. You could also add an empty_lines_only_at_beginning style, which wouldn't be as flexible, but might be simpler. (Or, you could call it landons_funky_empty_line_style. 😉)

@rrosenblum
Copy link
Contributor

I like the idea of adding configurations to Layout/EmptyLinesAroundClassBody rather than splitting this into 2 cops. However, the layout cops for spacing have individual cops for before and after so that would be more consistent with the rest of the code.

@jmks
Copy link
Contributor

jmks commented Oct 16, 2017

I've spiked out a solution, by adding an additional SupportedStyle for this cop:

Layout/EmptyLinesAroundClassBody:
  EnforcedStyle: no_empty_lines
  SupportedStyles:
    - empty_lines
    - empty_lines_except_namespace
    - empty_lines_special
    - no_empty_lines
    - beginning_only

I originally also had ending_only but after looking at the source, I was skeptical of that layout.

The actual changes would happen in lib/rubocop/cop/mixin/empty_lines_around_body.rb, so it would be easy to add this option to similar cops, like Layout/EmptyLinesAroundModuleBody.

This is a middle-ground approach compared to 1) splitting into 2 cops, and 2) introducing another config option(s) like allow_beginning_empty_lines, etc.

WDYT?

@LandonSchropp
Copy link
Author

That works for me! It solves my problem.

@johnnyshields
Copy link

👍

@johnnyshields
Copy link

johnnyshields commented Nov 2, 2017

@LandonSchropp I don't think the solution as proposed will work for RSpecs (describe, etc)

@paul
Copy link

paul commented Nov 4, 2017

Late to the party, but as long as we're considering modifying this Cop, I'd love it if I could exclude include and extend from the empty line required cop. That is, I like the empty line between class and the first def, but I prefer no line between class/module and the first include/extend.

Example:

class MyThing
  extend Forwardable

  attr_reader :thing 
  delegate %i( a b c ) => :thing

  def initialize(thing)
    @thing = thing
  end

end

I don't know if other class methods should also be excluded from the cop, like attr_reader or delegate, but I do feel that include and extend are "special", and should be permitted w/o the empty line.

Also happy to open a separate issue about it if that is preferred.

@paul
Copy link

paul commented Nov 4, 2017

Nevermind, now I see that was added as empty_lines_special in #3624.

@jonas054
Copy link
Collaborator

@jmks If you have a solution ready, a PR would be welcome.

@jmks
Copy link
Contributor

jmks commented Jan 21, 2018

Sorry, this fell off my radar. I'll make the PR in the next couple days.

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

No branches or pull requests

7 participants