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

Support EnforcedStyleForEmptyBraces for SpaceBeforeBlockBraces cop #4464

Merged

Conversation

palkan
Copy link
Contributor

@palkan palkan commented Jun 4, 2017

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:

# I don't want this one to be an offense
conf = Rack::Handler::Puma.config(->{}, @options)

Although for non-empty blocks there is always a space before the block.

This PR adds EnforcedStyleForEmptyBraces configuration parameter for Layout/SpaceBeforeBlockBracescop to allow handle empty braces differently.

@palkan palkan force-pushed the feature/space-before-block-empty-braces-style branch from 3185280 to 3cc9a26 Compare June 4, 2017 15:22
@@ -489,6 +489,9 @@ Layout/SpaceBeforeBlockBraces:
SupportedStyles:
- space
- no_space
SupportedStylesForEmptyBraces:
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Collaborator

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?

Copy link
Contributor Author

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').

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds reasonable.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 30, 2017

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...

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 30, 2017

@palkan
Copy link
Contributor Author

palkan commented Jul 4, 2017

it seems to me that this cop is not solving the problem it wants to.

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).

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 25, 2017

Yeah, I made a mistake I think.

Anyways, this has to be rebase on master - I see the changelog entry is in some weird place.

@palkan palkan force-pushed the feature/space-before-block-empty-braces-style branch 2 times, most recently from 1968793 to 41f0b29 Compare August 25, 2017 09:53
@palkan palkan force-pushed the feature/space-before-block-empty-braces-style branch from 41f0b29 to f594f4d Compare August 25, 2017 13:50
@palkan
Copy link
Contributor Author

palkan commented Aug 25, 2017

@bbatsov

this has to be rebase on master

Done. And green)

@bbatsov bbatsov merged commit 96a4b4d into rubocop:master Sep 1, 2017
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.

None yet

3 participants