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

[new rule] VerticalWhitespace rule #747

Merged
merged 2 commits into from Aug 22, 2016
Merged

Conversation

masters3d
Copy link
Contributor

@codecov-io
Copy link

codecov-io commented Aug 10, 2016

Current coverage is 89.49% (diff: 89.90%)

Merging #747 into master will increase coverage by 0.01%

@@             master       #747   diff @@
==========================================
  Files            74         75     +1   
  Lines          2480       2589   +109   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2219       2317    +98   
- Misses          261        272    +11   
  Partials          0          0          

Powered by Codecov. Last update fbe768e...854d1e6

@freak4pc
Copy link
Contributor

I'm not a fan of the naming. Seems a bit too complex for something that is practically a 'single_newlines' or just a 'newlines' rule perhaps. Not sure if my name her is any good but vertical spacing is pretty confusing imho.

@proth
Copy link

proth commented Aug 10, 2016

Same here. I had to read to code to get an idea what this rule is for.

@masters3d
Copy link
Contributor Author

@freak4pc Yeah, I am not in love with the name either but I didn't want to conflict with TrailingNewlineRule so I didn't want newLine in the name.

I really didn't find a consensus between these
*blank lines
*empty lines
*new lines
*blank space

so I picked vertical whitespace, another option could be vertical blank space

What about :
single_blank_line ?

@freak4pc
Copy link
Contributor

'newline_spacing' that perhaps could expect min and max but default to 1,1?

Or 'single_newlines' also seems fitting imo.

Thoughts on naming @jpsim ?

@masters3d
Copy link
Contributor Author

I am going to rename it to single_newline . Naming is hard.

@masters3d masters3d changed the title adds vertical whitespace rule #548 adds SingleNewLine correctable rule #548 Aug 11, 2016
@jpsim
Copy link
Collaborator

jpsim commented Aug 21, 2016

Thanks for your work on this @masters3d and your feedback @freak4pc!

I have two small suggestions: I think this should be named VerticalWhitespaceRule/vertical_whitespace and be configurable with a maxEmptyLines parameter that defaults to 1.

I don't think SingleNewLine is appropriate because for one empty line, there are two newline characters:

a

b

is actually a\n\nb.

@masters3d masters3d changed the title adds SingleNewLine correctable rule #548 [new rule] VerticalWhitespace rule #548 Aug 22, 2016
@masters3d masters3d changed the title [new rule] VerticalWhitespace rule #548 [new rule] VerticalWhitespace rule Aug 22, 2016
@masters3d
Copy link
Contributor Author

I changed back the name. I'd like to add maxEmptyLines configuration as another PR after this one lands.

"struct AAAA {}\n\n\n\n",
"class BBBB {}\n\n\n",
]
,
Copy link
Collaborator

Choose a reason for hiding this comment

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

comma should go on previous line

@jpsim
Copy link
Collaborator

jpsim commented Aug 22, 2016

@masters3d I have some comments.

I'd like to add maxEmptyLines configuration as another PR after this one lands.

Fine by me.

@jpsim jpsim merged commit 98d0375 into realm:master Aug 22, 2016
@jpsim
Copy link
Collaborator

jpsim commented Aug 22, 2016

Thanks for driving this forward @masters3d!

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.

Add rules around newlines
5 participants