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

Mixin grouping #5096

Merged
merged 2 commits into from
Nov 23, 2017
Merged

Mixin grouping #5096

merged 2 commits into from
Nov 23, 2017

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Nov 22, 2017

This is 3 fixes for the price of one.

First, this cop was doing multiple replacement rules for the same range. Only the first one was the correct one, so the tests were passing only because Rubocop silently swallows the ClobberingError of the erroneous subsequent replacements.

Second, the replacement was nuking anything in between two mixins

Thirdly, mixins with receivers could also create invalid code.

Note: Part of investigating parser's rewritter...

@marcandre marcandre force-pushed the mixin_grouping branch 2 times, most recently from c3dc72b to d059a71 Compare November 22, 2017 19:22
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 22, 2017

Apart from the failing build, the changes look good to me.

@marcandre
Copy link
Contributor Author

Hopefully build will pass now.

FWIW, I find it illuminating that my natural coding style seems to conflict so badly with rubocop's conventions, even though until this moment I considered myself an adept of the Ruby Style Guide.

I must admit that my interest in fixing errors in Rubocop went down because of this.

Adds support for split include/prepend/extend
Doesn't rely on cloberring errors being swallowed silently
nor on the order of nodes that `autocorrent` is called on.

In case of split calls, no effort to delete extra whitespace is made.
@bbatsov bbatsov merged commit 741a341 into rubocop:master Nov 23, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 23, 2017

FWIW, I find it illuminating that my natural coding style seems to conflict so badly with rubocop's conventions, even though until this moment I considered myself an adept of the Ruby Style Guide.

I didn't notice anything particularly off in your style? I assumed the failure was coming from some misalignment or some long line.

I must admit that my interest in fixing errors in Rubocop went down because of this.

I'm sad to hear this, but you know how it is - consistency within a codebase is important. Everything in RuboCop is configurable, what you're actually disliking are likely just own preferences (the project defaults) here.

@marcandre
Copy link
Contributor Author

I'm sad to hear this, but you know how it is - consistency within a codebase is important. Everything in RuboCop is configurable, what you're actually disliking are likely just own preferences (the project defaults) here.

Indeed 😄 I'm now officially strongly against the settings of the ABC metric, the method length metric and the line length metric.

It looks like I might not give the same importance to style over strict correctness. If I had to choose between which one of these metrics or #5094's validations that "are not needed, they are restricted unnecessarily", or if it's OK to silently swallow exceptions, my choice is clear.

BTW, I must applaud the responsive team, impressively quick feedback! 👏

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.

2 participants