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

update to rubocop 1.3.0 #221

Merged
merged 1 commit into from
Nov 13, 2020
Merged

update to rubocop 1.3.0 #221

merged 1 commit into from
Nov 13, 2020

Conversation

jmkoni
Copy link
Contributor

@jmkoni jmkoni commented Nov 12, 2020

Update rubocop from 1.2.0 to 1.3.0 enabling:

Copy link
Contributor

@twe4ked twe4ked left a comment

Choose a reason for hiding this comment

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

Thanks for doing all the work to keep this project ticking along.

Bit of a meta question, when we're doing these upgrades should we maybe take the default stance of not enabling new cops then open issues to discuss why they should be added?

The Style/NilLambda one for instance is probably not something that will come up very often and therefore probably doesn't have much impact on keeping code consistent. Compared to trailing commas or quote styles do. The more tiny rules we introduce the more nitpicky the tool will feel.

EDIT:

Just read this part of the `` PR:

It does not consider simple repeated bodies, like basic literals, variable references, constant references and simple method calls without arguments. As otherwise this would force to probably make code uglier. [CODE EXAMPLE]

This comment kind of points out that there might be reasons you want to have duplicate branches. My guess would be that the rules defined in that cop aren't going to account for all the edge cases.

@jmkoni
Copy link
Contributor Author

jmkoni commented Nov 12, 2020

So just a bit more information here: I don't choose these on my own. I put together all the new cops that are being added in a spreadsheet, give my recommendations, and talk it over with @searls. I do think we will want to follow a process like this once we hit 1.0, but, as of right now, that's a lot of conversations for us to monitor. There have been about 12 new cops added within the past 10 days, for example.

WRT the Lint/DuplicateBranch cop, we are definitely taking a gamble 😅 and recognize that it's one that we could be rolling back very soon!

@twe4ked
Copy link
Contributor

twe4ked commented Nov 12, 2020

Ahh good to know they're being discussed! That system sounds perfect for now. Thanks 🙏

There have been about 12 new cops added within the past 10 days, for example.

Wow.

@searls
Copy link
Contributor

searls commented Nov 12, 2020

I appreciate the sentiment in your suggestion, @twe4ked. In truth, Standard exists to be an anti-bikeshedding tool, and we try to accomplish that in several ways:

  1. Most obviously, standard can't be configured. It is what it is
  2. Standard should absorb the churn of rubocop changes (new cops, changed defaults, etc) should be largely absorbed by standard through thoughtful editorial review such that a normal rubocop user has to think about each new rubocop upgrade's changes, but a median standard user just has to upgrade standard and not worry about it
  3. When there is a problem, conflict or disagreement, Standard's repo should be a place to have that debate in public and wrestling with the needs of the whole community (as opposed to optimizing for the needs of each individual project team out there)

So of these, (1) is really how standard is already marketed and understood. What you're raising here is a presumed default for mode (3) (i.e. "hey you're turning a thing on and didn't discuss it first!"). I get that, and it'd make sense, but I think it's probably best to default to mode (2) for uncontroversial, probably-good-idea changes to rubocop for the sake of just taking advantage of its improvements without adding friction to standard's improvement.

Put differently, we don't want to have as a default workflow a debate over every single change. Surely there are plenty of generally agreeable rules that wouldn't foster much discussion (imagine if we had an open comment policy where we'd open an issue and wait 30 days for feedback before merge; it'd take a lot of time and admin and most issues would get zero comments). Rather, our goal is to take advantage of the one reliable thing about the Internet: if someone cares a lot about a particular thing Standard is doing they're going to tell us about it by opening a new issue. For that reason we're happy to turn some things on provisionally (like this if-else branch rule), see if it's as disruptive as we worry it might be, and then just roll it back in a patch release if it causes issue.

Make sense?

@twe4ked
Copy link
Contributor

twe4ked commented Nov 12, 2020

Yeah that makes a lot of sense, thanks for the detailed explanation. I incorrectly assumed that this PR was turning on all the new rules from the Rubocop update (feels like a silly assumption now 😅).

Thanks again, both of you!

@jmkoni jmkoni merged commit 09ddd16 into master Nov 13, 2020
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.

3 participants