-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support EnforcedStyleForEmptyBraces for SpaceBeforeBlockBraces cop #4464
Support EnforcedStyleForEmptyBraces for SpaceBeforeBlockBraces cop #4464
Conversation
3185280
to
3cc9a26
Compare
@@ -489,6 +489,9 @@ Layout/SpaceBeforeBlockBraces: | |||
SupportedStyles: | |||
- space | |||
- no_space | |||
SupportedStylesForEmptyBraces: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you are missing setting a default value for EnforcedStyleForEmptyBraces
. Based on the other usages, it looks like it should be set to no_space
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was made intentionally to avoid breaking changes. I'd prefer to fallback to EnforcedStyle
if EnforcedStyleForEmptyBraces
is missing.
@@ -71,4 +71,55 @@ | |||
expect_no_offenses('each{ puts }') | |||
end | |||
end | |||
|
|||
context 'with space before empty braces not allowed' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't some of the existing tests go under this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can put this context inside the 'EnforcedStyle' => 'space'
one, for example.
We can event extract these example into a shared examples group and include it into 'EnforcedStyle' => 'space'
(without explicit 'EnforcedStyleForEmptyBraces'
) and this context (with explicit 'EnforcedStyleForEmptyBraces'
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
Btw, it seems to me that this cop is not solving the problem it wants to. Doesn't the LambdaLiteral cop have support for this behavior? I don't see anything except lambda literals that would benefit from something like this... |
Sorry, don't understand what do you mean. The problem I want to solve is to accept these two examples: # no need of space before empty braces
fn = ->{}
# but always use space if lambda is not no-op
fn = -> { do_something } Currently, I cannot achieve this with the SpaceBeforeBlockBraces cop, so I have to disable it (although I don't want to). |
Yeah, I made a mistake I think. Anyways, this has to be rebase on |
1968793
to
41f0b29
Compare
41f0b29
to
f594f4d
Compare
Done. And green) |
I've started to integrate Rubocop into Puma development (puma/puma#1325) and found out that
SpaceBeforeBlockBraces
cannot handle empty braces differently (like, for example,SpaceInsideBlockBraces
does).There is a lot of code like this:
Although for non-empty blocks there is always a space before the block.
This PR adds
EnforcedStyleForEmptyBraces
configuration parameter forLayout/SpaceBeforeBlockBraces
cop to allow handle empty braces differently.