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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Rubocop 0.57.2 #4300

Merged
merged 2 commits into from Jul 1, 2018
Merged

Upgrade to Rubocop 0.57.2 #4300

merged 2 commits into from Jul 1, 2018

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented Jun 7, 2018

  • 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. N/A. Brew style rubocop behavior is not tested; I don't think.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Any interest in upgrading to Rubocop 0.57.1? I thought it might fix an error I was running in to with brew style --rspec. It didn't, but I went and did the work for it anyway (including style fixes) before realizing that. Do we care about being on a newer version of Rubocop?

@ilovezfs
Copy link
Contributor

ilovezfs commented Jun 7, 2018

I'd like to see the EOS is not aligned with thing whitelisted, at least for core, because it would affect the caveats and plist formatting unnecessarily and rubocop seems to invent some new heredoc pedantry every version.

@MikeMcQuaid
Copy link
Member

@reitermarkus rubocop-cask needs updated here, thanks. Would be good maybe do handle merging that into Homebrew/brew next on the GSoC after search?

@apjanke will need a rebase.

I'd like to see the EOS is not aligned with thing whitelisted, at least for core, because it would affect the caveats and plist formatting unnecessarily and rubocop seems to invent some new heredoc pedantry every version.

I disagree here; the changes that have been made here seem to be more consistent and readable (and more like other Ruby blocks) in their indentation style. I do think we should do the usual "fix Homebrew/homebrew-core styles" PR before this is merged and discuss things there.

@MikeMcQuaid
Copy link
Member

I do think we should do the usual "fix Homebrew/homebrew-core styles" PR before this is merged and discuss things there.

Have opened Homebrew/homebrew-core#28754 to discuss.

@ghost ghost assigned reitermarkus Jun 20, 2018
@ghost ghost added the in progress Maintainers are working on this label Jun 20, 2018
@MikeMcQuaid
Copy link
Member

@apjanke Any chance of a rebase here? Thanks!

@apjanke
Copy link
Contributor Author

apjanke commented Jun 30, 2018

Rebased!

reitermarkus
reitermarkus previously requested changes Jun 30, 2018
View reviewed changes
Library/Homebrew/cask/lib/hbc/locations.rb Outdated
@@ -0,0 +1,31 @@
module Hbc
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't exist anymore in master.

Library/Homebrew/constants.rb Outdated
@@ -1,5 +1,5 @@
# frozen_string_literal: true

# RuboCop version used for `brew style` and `brew cask style`
HOMEBREW_RUBOCOP_VERSION = "0.55.0"
HOMEBREW_RUBOCOP_VERSION = "0.57.1"
HOMEBREW_RUBOCOP_CASK_VERSION = "~> 0.19.0" # has to be updated when RuboCop version changes
Copy link
Member

Choose a reason for hiding this comment

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

Bump to 0.20.0.

@apjanke
Copy link
Contributor Author

apjanke commented Jun 30, 2018

Amended to address @reitermarkus's comments.

reitermarkus
reitermarkus reviewed Jun 30, 2018
View reviewed changes
Library/Homebrew/constants.rb Outdated
@@ -1,5 +1,5 @@
# frozen_string_literal: true

# RuboCop version used for `brew style` and `brew cask style`
HOMEBREW_RUBOCOP_VERSION = "0.55.0"
HOMEBREW_RUBOCOP_CASK_VERSION = "~> 0.19.0" # has to be updated when RuboCop version changes
HOMEBREW_RUBOCOP_VERSION = "0.57.1"
Copy link
Member

Choose a reason for hiding this comment

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

0.57.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended.

@apjanke apjanke changed the title Upgrade to Rubocop 0.57.1 Upgrade to Rubocop 0.57.2 Jun 30, 2018
reitermarkus
reitermarkus reviewed Jun 30, 2018
View reviewed changes
Library/Homebrew/.rubocop.yml Outdated
@@ -72,6 +77,11 @@ Naming/PredicateName:
Naming/UncommunicativeMethodParamName:
Enabled: false

# I'm not sure how to correct these, and seems to get false positives on
# modifiers used on symbols of methods
Copy link
Member

Choose a reason for hiding this comment

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

Change this comment to reference rubocop/rubocop#5953.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@MikeMcQuaid MikeMcQuaid dismissed reitermarkus’s stale review July 1, 2018 08:08

All comments addressed

@MikeMcQuaid
Copy link
Member

Thanks @apjanke!

@MikeMcQuaid MikeMcQuaid merged commit 61a8c4d into Homebrew:master Jul 1, 2018
@ghost ghost removed the in progress Maintainers are working on this label Jul 1, 2018
@apjanke
Copy link
Contributor Author

apjanke commented Jul 1, 2018

Yay!

@apjanke apjanke deleted the new-rubocop-version branch July 1, 2018 08:11
@lock lock bot added the outdated PR was locked due to age label Jul 31, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reitermarkus reitermarkus reitermarkus left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees

@reitermarkus reitermarkus

Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@apjanke @ilovezfs @MikeMcQuaid @reitermarkus