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

Clean up code style and remove `.rubocop_todo.yml`. #3278

Merged
merged 1 commit into from Oct 8, 2017

Conversation

Projects
None yet
3 participants
@reitermarkus
Member

reitermarkus commented Oct 6, 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?
@MikeMcQuaid

A few nits but nothing major or blocking. Make sure when you merge this you'll be at a computer for a while (both a few hours and over the following few days) to fix any bugs that will creep up as a result. Nice work!

Max: 5
Metrics/ModuleLength:
Max: 360

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Good idea to just setting these to high values. Mind doing that for line length, too? Would be good to get it down eventually but at least stop it growing for now.

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Good idea to just setting these to high values. Mind doing that for line length, too? Would be good to get it down eventually but at least stop it growing for now.

@@ -54,8 +54,7 @@ def tap
quiet: ARGV.quieter?
rescue TapRemoteMismatchError => e
odie e
rescue TapAlreadyTappedError, TapAlreadyUnshallowError
# Do nothing.
rescue TapAlreadyTappedError, TapAlreadyUnshallowError # rubocop:disable Lint/HandleExceptions

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Not worth keeping the comment and returning nil instead?

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Not worth keeping the comment and returning nil instead?

@@ -186,7 +186,7 @@ def atomic_write(content)
begin
tf.chown(uid, gid)
tf.chmod(old_stat.mode)
rescue Errno::EPERM
rescue Errno::EPERM # rubocop:disable Lint/HandleExceptions

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Worth returning nil here and skipping the `disable (and everywhere below)?

@MikeMcQuaid

MikeMcQuaid Oct 8, 2017

Member

Worth returning nil here and skipping the `disable (and everywhere below)?

@ilovezfs

This comment has been minimized.

Show comment
Hide comment
@ilovezfs

ilovezfs Oct 8, 2017

Contributor

@reitermarkus maybe should rebase now that #3284 is merged?

Contributor

ilovezfs commented Oct 8, 2017

@reitermarkus maybe should rebase now that #3284 is merged?

@reitermarkus

This comment has been minimized.

Show comment
Hide comment
@reitermarkus

reitermarkus Oct 8, 2017

Member

I'll leave the comments for now. I think it's more obvious to see the difference between explicitly returning nil and returning it just to silence RuboCop.

Member

reitermarkus commented Oct 8, 2017

I'll leave the comments for now. I think it's more obvious to see the difference between explicitly returning nil and returning it just to silence RuboCop.

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

3 checks passed

codecov/patch 76.79% of diff hit (target 67.55%)
Details
codecov/project 68.21% (+0.66%) compared to 91ab116
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@reitermarkus reitermarkus deleted the reitermarkus:code-style branch Oct 8, 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.