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

RuboCop changes. #1128

Merged
merged 4 commits into from Sep 27, 2016

Conversation

Projects
None yet
5 participants
@reitermarkus
Copy link
Member

reitermarkus commented Sep 24, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Further RuboCop fixes. Also updated the configuration to include more of Cask's rules.

@reitermarkus reitermarkus force-pushed the reitermarkus:rubocop branch 3 times, most recently from f8a1dd0 to a27d4ee Sep 25, 2016

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 25, 2016

NO.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 25, 2016

@zmwangx @ilovezfs Those comments are extremely unhelpful. Please provide reasons for disagreement or stay silent. It's bad enough when comments like this come from users but I expect better from our maintainers.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 25, 2016

I suspect if the prior breaking changes are fixed first, more useful reviews may follow.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 25, 2016

I suspect if the prior breaking changes are fixed first, more useful reviews may follow.

I suspect if someone had created an issue or, dare I say it, a pull-request then the prior breaking changes would have been fixed already.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 25, 2016

I suspect if someone had created an issue or, dare I say it, a pull-request then the prior breaking changes would have been fixed already.

Already suggested a diff on slack hours ago and explained why I didn't open a PR for it. But sure.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 25, 2016

The multi-space alignments are so laughable I expect anyone who actually review the changes to immediately roll their eyes.

HOMEBREW_INTERNAL_COMMAND_ALIASES = {
  "ls"          => "list",
  "homepage"    => "home",
  "-S"          => "search",
  "up"          => "update",
  "ln"          => "link",
  "instal"      => "install", # gem does the same
  "rm"          => "uninstall",
  "remove"      => "uninstall",
  "configure"   => "diy",
  "abv"         => "info",
  "dr"          => "doctor",
  "--repo"      => "--repository",
  "environment" => "--env",
  "--config"    => "config",
}.freeze
      SYMBOLS = {
        sierra:        "10.12",
        el_capitan:    "10.11",
        yosemite:      "10.10",
        mavericks:     "10.9",
        mountain_lion: "10.8",
        lion:          "10.7",
        snow_leopard:  "10.6",
        leopard:       "10.5",
        tiger:         "10.4",
      }.freeze

Now, to add or remove one key, you potentially need to manually realign all of the values. Seriously?

You may not agree with the point but they are not pointless. They are investment in a consistent style which makes it easier to automatically review contributed PRs and lets people know what our style is rather than it being arbitrary.

More importantly, who decided that we should follow a "consistent style" like

   Style/AlignHash:
   EnforcedHashRocketStyle: table
   EnforcedColonStyle: table
   Exclude:
     - 'Taps/**/*'

Did anyone ask for this (sorry, maybe I missed the discussion)? Or is this just @reitermarkus's personal preference somehow?

And if I didn't call it out (although I called it out in an unhelpful way, at least in this thread, for which I apologize) this would have been merged very soon like every other of these RuboCop PRs, and suddenly the entire team had to conform to some random style decisions made by @reitermarkus? Not to mention some claimed style changes are not only stylistic? Rather than hastily merging these after getting one approval, shouldn't we need the consensus of the team before moving forward on things like multi-space alignments?

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Sep 25, 2016

Did anyone ask for this (sorry, maybe I missed the discussion)? Or is this just @reitermarkus's personal preference somehow?

I am trying to merge Homebrew's style with Cask's, so naturally I'm trying to incorporate some of Cask's rules into Homebrew's style, and vice-versa, and not completely eliminate one of them.

Also, I would argue that having an ugly diff is better than having hard-to-read code.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 25, 2016

The multi-space alignments are so laughable I expect anyone who actually review the changes to immediately roll their eyes.

Hyperbole does not help. Plenty of Ruby projects use this style. I also agree we should not take this particular style change (my review comment from earlier got eaten, somehow) as I also agree that requiring the indentation to be consistent causes diff-inflation.

Did anyone ask for this (sorry, maybe I missed the discussion)? Or is this just @reitermarkus's personal preference somehow?

Even if it is his personal preference the correct way is to disagree with that preference and explain why rather than to attack either the person or the higher-level project (getting our Ruby style to be consistent with Rubocop).

Rather than hastily merging these after getting one approval, shouldn't we need the consensus of the team before moving forward on things like multi-space alignments?

We don't need consensus for things that can be trivially changed back to how they were. I'm happy to get more views on these but previously I've been the only person providing any review, pre or post merge, on these Homebrew/brew PRs. If you'd like to be more involved and would like us to take more time before merging I'm happy to have your review and thoughts on these and let me know what an acceptable amount of time is to give you to review before merging.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 25, 2016

Already suggested a diff on slack hours ago and explained why I didn't open a PR for it. But sure.

Sorry about that. This is my fault as I've not been explicit about it before but: there's too much chat in Slack and it's not publicly available so it's better to assume messages there have not been read and to cross-post thoughts into issues/PRs/etc.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Made some comments. Sorry, tried to make them earlier but they got eaten.

Style/Alias:
EnforcedStyle: prefer_alias

Style/AlignHash:

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2016

Member

I reckon it's worth not having this because of the realignment required when different length keys are added to a large existing hash and the expanding diff that results.

Exclude:
- 'Taps/**/*'

Style/IfUnlessModifier:

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2016

Member

I think unless this can be somehow told "don't put this on the same line if it makes the line exceed >80 characters" then it's worth omitting.

This comment has been minimized.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2016

Member

Gotcha. Yeh, that'd be good to use 👍

This comment has been minimized.

@reitermarkus

reitermarkus Sep 25, 2016

Member

Well, it is already in use, it being the default, but apparently it does not work in the other direction: rubocop-hq/rubocop#3534

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2016

Member

🆒, probably worth disabling until then.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 27, 2016

Member

@reitermarkus Thoughts on this? Did you just update the changes to that they match this?

This comment has been minimized.

@reitermarkus

reitermarkus Sep 27, 2016

Member

I left this as-is, because disabling doesn't change anything. Corrected two lines manually though which were > 80 characters, which will be caught automatically once this is fixed.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 27, 2016

Member

Cool, thanks.

@@ -4,7 +4,7 @@

module Homebrew
module Cleanup
@@disk_cleanup_size = 0
@disk_cleanup_size = 0

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Sep 25, 2016

Member

Just to check: does brew cleanup still produce the same results afterwards and is this equivalent?

This comment has been minimized.

@reitermarkus

reitermarkus Sep 25, 2016

Member

Still working as expected.

This comment has been minimized.

@MikeMcQuaid

@reitermarkus reitermarkus force-pushed the reitermarkus:rubocop branch from a27d4ee to 982ae92 Sep 25, 2016

@reitermarkus reitermarkus force-pushed the reitermarkus:rubocop branch from 982ae92 to b114db1 Sep 25, 2016

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 25, 2016

I do think we need to be careful with these mass changes, given our test coverage is still a work in progress. Introducing real breakage through style fixes certainly isn't a desired side-effect, and if tests aren't catching that the tests obviously aren't comprehensive enough.

I don't think any comment here or in #1125 were intending to set fire to @reitermarkus personally. The writing style was a little blunt, but I think it mostly stemmed from frustration that style changes are introducing breakage, style changes which to be fair haven't really been explained or discussed all that much, rather than setting out to "get" anyone. Almost the entire team, myself very much included, are guilty of using frustrated voices at times. We can all do better there.

It would be useful if the changes were explained a little better, so people don't have to go hunting around Rubocop upstream for examples on the before/after.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 25, 2016

style changes are introducing breakage, style changes which to be fair haven't really been explained or discussed all that much

Just to be more explicit about these: we have a .rubocop_todo.yml file and Cask has it's own .rubocop.yml file and they should both be worked on until we have no TODOs and a single, shared file with shared style between Homebrew, Cask and (where appropriate and not excluded) Formulae and Taps. This is useful because it makes it easier to contribute and read code that's using a single style and almost all Rubocop checks are to make Ruby code more readable and thus easier to work with. Readability is the foundation on which security, performance, correctness etc. are built.

There will be bugs along the way and the best ways to combat this are for me and/or @reitermarkus to not merge these PRs too soon before a tag or us going out/to bed to that we can jump to fix issues when needed.

@MikeMcQuaid MikeMcQuaid merged commit 4488edd into Homebrew:master Sep 27, 2016

4 checks passed

codecov/patch 82.35% of diff hit (target 65.75%)
Details
codecov/project 65.78% (+0.02%) compared to 6e57355
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@reitermarkus reitermarkus deleted the reitermarkus:rubocop branch Sep 27, 2016

@DomT4 DomT4 referenced this pull request Sep 27, 2016

Closed

Rubocop tap problems. #1168

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.