switch SpaceAfterPunctuation mixin to use SpaceInsideHashLiteralBraces config

[ptarjan]

Having a trailing , in a Block won't compile and having a trailing ; in a hash won't compile. So they should use their respective configs.

Before submitting a PR make sure the following are checked:

• Wrote good commit messages.
• Used the same coding conventions as the rest of the project.
• Feature branch is up-to-date with master (if not - rebase it)
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• All tests are passing.
• The new code doesn't generate RuboCop offenses.
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences.

[ptarjan]

ping @bbatsov

[ptarjan]

mild ping again @bbatsov

[bbatsov]

Sorry, @ptarjan. I'm crazy busy and I don't have much time for RuboCop these days.

[ptarjan]

@bbatsov bummer :( Well I just want to make sure to get this in before you cut another release. Is there another maintainer you can deputize?

[bbatsov]

You can ask @jonas054 to review this PR. Next week should be less busy for me and I'll try to cut a new release then.

[bbatsov]

Btw, what exactly is the purpose of the PR - it's not immediately obvious to me.

[ptarjan]

@bbatsov Our config has different values for Style/SpaceInsideBlockBraces and Style/SpaceInsideHashLiteralBraces so our , spacing was getting into an infinite loop because the SpaceAfterPunctuation Cop was using one config, but then the SpaceInsideHashLiteralBraces was trying to correct it the other way.

[bbatsov]

I understand your point, but I don't think this is the right approach. Maybe we should just raise an error when someone has set incompatible settings - after all the two cops tackle different issues. Or we should add some special handling for such edge cases. In general we aim to avoid coupling between cops unless they are very tightly linked.

[ptarjan]

How are my two cops inconsistent? I like to have spaces for hashes and not have spaces for lambdas. I like that it differentiates them in the code.

Right now this other cop tries to use one of the configs for spaces after a comma and a semicolon. I was hanging it to use the other config for spaces after a semicolon.

Sent from my iPhone

On Apr 22, 2016, at 12:02 PM, Bozhidar Batsov notifications@github.com wrote:

I understand your point, but I don't think this is the right approach. Maybe we should just raise an error when someone has set incompatible settings - after all the two cops tackle different issues. Or we should add some special handling for such edge cases. In general we aim to avoid coupling between cops unless they are very tightly linked.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

[bbatsov]

How are my two cops inconsistent? I like to have spaces for hashes and not have spaces for lambdas. I like that it differentiates them in the code.

One deals with blocks, one deals with hash literals - I don't see why they should share their configuration.

[ptarjan]

Well the SpaceAfterComma and the SpaceAfterSemicolon cops use the configuration from SpaceInsideBlockBraces right now. I was wanting the comma one to use SpaceInsideHashLiteralBraces

[bbatsov]

Guess I missed this. @jonas054 Do you remember why you did it like this?

[ptarjan]

Ping. I'd be sad if 0.40 went out without this.

[bbatsov]

Don't worry - I'm enjoying my California vacation so much, that I totally forgot about OSS. Guess 0.40 won't happen for a few more days.

[ptarjan]

If you're anywhere near San Francisco during your vacation and want to see the Stripe office and have a free lunch, let me know :)

[jonas054]

@ptarjan I have finally started reviewing your changes. Sorry about the delay.

One thing I've noticed is that the changed spec passes even with the current master implementation of the files under lib. So please add some specs that fail on master and demonstrate the problem. For my understanding of the problem, I'd prefer a spec example in spec/rubocop/cli/cli_autocorrect_spec.rb that shows how auto-correct changes code back and forth.

[jonas054]

I can't find that we've used this technique before, checking "calling an abstract method". It's code that's impossible (more or less) to cover in tests. I don't think we need it. We'll find out in testing if any implementation is missing, even without it.

[ptarjan]

I added a test there that fails on master. Do you still want the matrix test I added?

[ptarjan]

@jonas054 thanks for the review

[jonas054]

Yes I think the individual cop specs are useful and should test each cop completely. The CLI specs are good for interactions between cops.

But actually, it doesn't make much sense to configure SpaceInsideHashLiteralBraces when testing SpaceAfterSemicolon since that configuration is only read by the SpaceAfterComma cop. So remove that part and add it to the specs for SpaceAfterComma instead.

You have a line that's too long in cli_autocorrect_spec.rb BTW. And you need to add something in the changelog, but I suspect you knew that.

[ptarjan]

@jonas054 done. Thanks for the quick response.

[jonas054]

👍 Looks good!