Skip to content

Rubocop: 0.50.0 and Ruby 2.3 #3183

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
Merged
MikeMcQuaid merged 3 commits into from
Sep 25, 2017
Merged

Rubocop: 0.50.0 and Ruby 2.3 #3183

MikeMcQuaid merged 3 commits into from
Sep 25, 2017

Conversation

@MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Sep 21, 2017

Also fix all cop renames, disable some cops and fix all warnings.

Want to also fix any/all Homebrew/core warnings first before merging this.

ilovezfs
ilovezfs reviewed Sep 21, 2017
View reviewed changes
Library/Homebrew/brew.rb Outdated
Copy link
Contributor

@ilovezfs ilovezfs Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this different from unless possible_tap&.installed??

Copy link
Member Author

@MikeMcQuaid MikeMcQuaid Sep 21, 2017

Choose a reason for hiding this comment

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

It's the same but it, like me, doesn't like unless ... else

Copy link
Contributor

@ilovezfs ilovezfs Sep 21, 2017

Choose a reason for hiding this comment

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

ah

ilovezfs
ilovezfs reviewed Sep 21, 2017
View reviewed changes
Library/Homebrew/caveats.rb Outdated
Copy link
Contributor

@ilovezfs ilovezfs Sep 21, 2017

Choose a reason for hiding this comment

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

are the parentheses still needed?

Copy link
Member Author

@MikeMcQuaid MikeMcQuaid Sep 21, 2017

Choose a reason for hiding this comment

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

Removed.

ilovezfs
ilovezfs reviewed Sep 21, 2017
View reviewed changes
Library/Homebrew/cmd/prune.rb Outdated
Copy link
Contributor

@ilovezfs ilovezfs Sep 21, 2017

Choose a reason for hiding this comment

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

oh how gross. can we opt out of this?

Copy link
Member Author

@MikeMcQuaid MikeMcQuaid Sep 21, 2017

Choose a reason for hiding this comment

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

Use Python?

Copy link
Contributor

@ilovezfs ilovezfs Sep 21, 2017

Choose a reason for hiding this comment

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

you first

reitermarkus
reitermarkus reviewed Sep 21, 2017
View reviewed changes
Library/Homebrew/extend/ENV/std.rb Outdated
Copy link
Member

@reitermarkus reitermarkus Sep 21, 2017

Choose a reason for hiding this comment

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

to_i should be inside the parentheses.

DomT4
DomT4 reviewed Sep 22, 2017
View reviewed changes
Library/.rubocop.yml Outdated
Copy link
Contributor

@DomT4 DomT4 Sep 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for catching this. My $EDITOR's Rubocop plugin has already been driving me up the wall over EOS stuff.

Edit - Up the wall, not up to the wall.

Copy link
Contributor

@ilovezfs ilovezfs Sep 22, 2017

Choose a reason for hiding this comment

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

You can thank me for the fit I pitched about it lol

Copy link
Contributor

@DomT4 DomT4 Sep 22, 2017

Choose a reason for hiding this comment

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

I don't understand what the issue is with EOS to be honest. It'd seem silly to get ultra-specific inside formulae at least.

Copy link
Contributor

@ilovezfs ilovezfs Sep 22, 2017

Choose a reason for hiding this comment

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

rubocop/rubocop#4467

I get the sense that whatever conversations there were about this didn't really occur in the open.

Copy link
Member

@reitermarkus reitermarkus Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

One nice thing is that in TextMate you get nested syntax highlighting inside heredocs if you use e.g. RUBY, but if it's just text, I'm also in favour of EOS.

@reitermarkus
Copy link
Member

reitermarkus commented Sep 22, 2017 •
edited
Loading

You can bump HOMEBREW_RUBOCOP_CASK_VERSION to 0.14.2 now.

@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 23, 2017

It's fitting that the only offense in all of core is

== Formula/gl2ps.rb ==
C: 61: 39: Use File.size("test.eps").positive? instead of File.size("test.eps") > 0.

4357 files inspected, 1 offense detected

@reitermarkus
Copy link
Member

reitermarkus commented Sep 23, 2017

RuboCop should at least suggest nonzero? in this case. 😜

@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 23, 2017

git bisect led me here.

I'm getting

iMac-TMP:homebrew-core joe$ brew pull https://github.com/Homebrew/homebrew-core/pull/18474
==> Fetching patch 
Patch: https://github.com/Homebrew/homebrew-core/pull/18474.patch
######################################################################## 100.0%
==> Applying patch
Applying: libhttpseverywhere 0.6.0
Warning: libhttpseverywhere has a bottle: do you need to update it with --bottle?
==> Patch closes issue #18474
Warning: Nonstandard bump subject: libhttpseverywhere 0.6.0
Warning: Subject should be: libhttpseverywhere  (new formula)
==> Patch changed:
 Formula/libhttpseverywhere.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

which does not occur without this pulled.

The message

Warning: Subject should be: libhttpseverywhere  (new formula)

is bogus.

@MikeMcQuaid MikeMcQuaid merged commit a589303 into Homebrew:master Sep 25, 2017
@MikeMcQuaid MikeMcQuaid deleted the rubocop-upgrade branch September 25, 2017 20:29
@commitay commitay mentioned this pull request Sep 25, 2017
Closed
@ilovezfs ilovezfs mentioned this pull request Sep 26, 2017
Merged
5 tasks
@ilovezfs
Copy link
Contributor

ilovezfs commented Oct 8, 2017

It seems 9eb51db broke the description audit. See #3286.

ilovezfs added a commit to ilovezfs/brew that referenced this pull request Nov 2, 2017
@ilovezfs
formula: fix safe navigation bug
3769bcc 
Safe navigation needs to be chained to preserve equivalence.

Fixes a bug introduced by 01e9ec9 in Homebrew#3183.
@ilovezfs ilovezfs mentioned this pull request Nov 2, 2017
Merged
5 tasks
@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.

Reviewers

@reitermarkus reitermarkus reitermarkus left review comments

+3 more reviewers

@ilovezfs ilovezfs ilovezfs approved these changes

@DomT4 DomT4 DomT4 left review comments

@JCount JCount JCount approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MikeMcQuaid @reitermarkus @ilovezfs @DomT4 @JCount