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 0.51 #3349

Merged
merged 9 commits into from Oct 24, 2017

Conversation

Projects
None yet
4 participants
@reitermarkus
Member

reitermarkus commented Oct 21, 2017

  • 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?

Update RuboCop to 0.51 and fix/refactor specs.

Versioned formulae should not use `conflicts_with`.
Use `keg_only :versioned_formula` instead.
EOS
MSG = "Versioned formulae should not use `conflicts_with`. " \

This comment has been minimized.

@DomT4

DomT4 Oct 21, 2017

Contributor

Struggling to see how a bunch of backslashes with heavier indentation are any more readable than an EOS block, personally 🙈.

@DomT4

DomT4 Oct 21, 2017

Contributor

Struggling to see how a bunch of backslashes with heavier indentation are any more readable than an EOS block, personally 🙈.

This comment has been minimized.

@reitermarkus

reitermarkus Oct 22, 2017

Member

It needs to be a single line for it to work with expect_offense, that's the reason. EOS was two lines.

@reitermarkus

reitermarkus Oct 22, 2017

Member

It needs to be a single line for it to work with expect_offense, that's the reason. EOS was two lines.

This comment has been minimized.

@DomT4

DomT4 Oct 22, 2017

Contributor

Does something like .split("\n") not work?

@DomT4

DomT4 Oct 22, 2017

Contributor

Does something like .split("\n") not work?

This comment has been minimized.

@reitermarkus

reitermarkus Oct 22, 2017

Member

No, it only supports a single line. Look at one of the specs where expect_offense is used. It parses the lines with ^^^^^^^^ and matches them.

@reitermarkus

reitermarkus Oct 22, 2017

Member

No, it only supports a single line. Look at one of the specs where expect_offense is used. It parses the lines with ^^^^^^^^ and matches them.

This comment has been minimized.

@DomT4

DomT4 Oct 22, 2017

Contributor

I think this is a fairly profound regression on readability, and honestly something that would deter me from wanting to spend much time with these files, but it is what it is given it's a change forced by the Rubocop side. Mike's approved it & I stepped away, so I'll go quietly. Just a bit of a shame to lose the current style.

@DomT4

DomT4 Oct 22, 2017

Contributor

I think this is a fairly profound regression on readability, and honestly something that would deter me from wanting to spend much time with these files, but it is what it is given it's a change forced by the Rubocop side. Mike's approved it & I stepped away, so I'll go quietly. Just a bit of a shame to lose the current style.

This comment has been minimized.

@reitermarkus

reitermarkus Oct 24, 2017

Member

That doesn't seem more readable to me.

@reitermarkus

reitermarkus Oct 24, 2017

Member

That doesn't seem more readable to me.

This comment has been minimized.

@ilovezfs

ilovezfs Oct 24, 2017

Contributor

Personally I think some junk on the first line is better than quotes and backslashes at the end of every line but I will leave that between you and Dom.

@ilovezfs

ilovezfs Oct 24, 2017

Contributor

Personally I think some junk on the first line is better than quotes and backslashes at the end of every line but I will leave that between you and Dom.

This comment has been minimized.

@DomT4

DomT4 Oct 24, 2017

Contributor

I think @DomT4 meant something like

Yeah I didn't actually do testing, I was just tossing ideas off of the top of my head in terms of could we not do this literally any other way.

That doesn't seem more readable to me.

I agree it's not brilliant but at least from my point of view there is nothing worse than backslashes for readability. For me there is an appreciable difference between:

blah "Polly ate three crackers and two of those had butter on " \
     "except one had salted butter and the other unsalted. " \
     "Polly wasn't sure why she was documenting her lunch to " \
     "such a degree but wondered if she should join Instagram " \
     "where documenting lunch seems to be very in vogue."

And:

blah <<~EOS.tr("\n", " ").strip.freeze
  Polly ate three crackers and two of those had butter on
  except one had salted butter and the other unsalted.
  Polly wasn't sure why she was documenting her lunch to
  such a degree but wondered if she should join Instagram
  where documenting lunch seems to be very in vogue.
EOS

The latter has a flow that isn't punched in the face by having to check for backslashes constantly. The longer the block of text the worse it becomes. And the latter also avoids that grim necessity for whitespace before the closing " on each line except the final one.

@DomT4

DomT4 Oct 24, 2017

Contributor

I think @DomT4 meant something like

Yeah I didn't actually do testing, I was just tossing ideas off of the top of my head in terms of could we not do this literally any other way.

That doesn't seem more readable to me.

I agree it's not brilliant but at least from my point of view there is nothing worse than backslashes for readability. For me there is an appreciable difference between:

blah "Polly ate three crackers and two of those had butter on " \
     "except one had salted butter and the other unsalted. " \
     "Polly wasn't sure why she was documenting her lunch to " \
     "such a degree but wondered if she should join Instagram " \
     "where documenting lunch seems to be very in vogue."

And:

blah <<~EOS.tr("\n", " ").strip.freeze
  Polly ate three crackers and two of those had butter on
  except one had salted butter and the other unsalted.
  Polly wasn't sure why she was documenting her lunch to
  such a degree but wondered if she should join Instagram
  where documenting lunch seems to be very in vogue.
EOS

The latter has a flow that isn't punched in the face by having to check for backslashes constantly. The longer the block of text the worse it becomes. And the latter also avoids that grim necessity for whitespace before the closing " on each line except the final one.

This comment has been minimized.

@reitermarkus

reitermarkus Oct 24, 2017

Member

It may be worth it for many lines, but not for only 2 as in this case.

@reitermarkus

reitermarkus Oct 24, 2017

Member

It may be worth it for many lines, but not for only 2 as in this case.

This comment has been minimized.

@DomT4

DomT4 Oct 24, 2017

Contributor

But then you're simply introducing inconsistency across a codebase 🙈.

@DomT4

DomT4 Oct 24, 2017

Contributor

But then you're simply introducing inconsistency across a codebase 🙈.

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 23, 2017

Contributor

@reitermarkus I notice that you've changed several structured expectations like

      expected_offenses = [{  message: "sha256 should be 64 characters",
                              severity: :convention,
                              line: 5,
                              column: 12,
                              source: source },
                           {  message: "sha256 should be 64 characters",
                              severity: :convention,
                              line: 9,
                              column: 14,
                              source: source }]

into exact string matches that involve things like

 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is this a mandatory change dictated by 0.51 or something you're electing to do, that we could choose not to do? I think we're going backwards in terms of maintainability and readability moving from the former to the latter, even if it is fewer lines.

Contributor

ilovezfs commented Oct 23, 2017

@reitermarkus I notice that you've changed several structured expectations like

      expected_offenses = [{  message: "sha256 should be 64 characters",
                              severity: :convention,
                              line: 5,
                              column: 12,
                              source: source },
                           {  message: "sha256 should be 64 characters",
                              severity: :convention,
                              line: 9,
                              column: 14,
                              source: source }]

into exact string matches that involve things like

 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is this a mandatory change dictated by 0.51 or something you're electing to do, that we could choose not to do? I think we're going backwards in terms of maintainability and readability moving from the former to the latter, even if it is fewer lines.

@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 23, 2017

Member

Is this a mandatory change dictated by 0.51

Not really mandatory, but none of the RuboCop specs is using the old way anymore, so this is basically depending on undocumented behaviour.

Member

reitermarkus commented Oct 23, 2017

Is this a mandatory change dictated by 0.51

Not really mandatory, but none of the RuboCop specs is using the old way anymore, so this is basically depending on undocumented behaviour.

@@ -1,5 +1,5 @@
# frozen_string_literal: true
# RuboCop version used for `brew style` and `brew cask style`
HOMEBREW_RUBOCOP_VERSION = "0.50.0"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 24, 2017

Member

Can we put these in the Gemfile instead now?

@MikeMcQuaid

MikeMcQuaid Oct 24, 2017

Member

Can we put these in the Gemfile instead now?

This comment has been minimized.

@reitermarkus

reitermarkus Oct 24, 2017

Member

Not in this PR.

@reitermarkus

reitermarkus Oct 24, 2017

Member

Not in this PR.

@MikeMcQuaid

This comment has been minimized.

Show comment
Hide comment
@MikeMcQuaid

MikeMcQuaid Oct 24, 2017

Member

We had expect_offense defined ourselves before, but now RuboCop's rspec support includes a expect_offense helper method, which they use for all of their specs.

I'm in favour of using RuboCop's version rather than ours here even if this means a slightly less structured approach.

Member

MikeMcQuaid commented Oct 24, 2017

We had expect_offense defined ourselves before, but now RuboCop's rspec support includes a expect_offense helper method, which they use for all of their specs.

I'm in favour of using RuboCop's version rather than ours here even if this means a slightly less structured approach.

@reitermarkus reitermarkus merged commit 8be34a9 into Homebrew:master Oct 24, 2017

3 checks passed

codecov/patch 81.81% of diff hit (target 68.06%)
Details
codecov/project 68.6% (+0.54%) compared to cd61430
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reitermarkus reitermarkus deleted the reitermarkus:rubocop-0.51 branch Oct 24, 2017

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

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