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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #4843] Fix shadow rescued Exceptions of same error code #4844

Merged

Conversation

Projects
None yet
4 participants
@koic
Copy link
Member

commented Oct 7, 2017

This PR fixes #4843.
馃摑 System dependent error code depends on runtime environment.

This PR also refactor against the following offenses arising from this change.
1db316c

% rubocop lib/rubocop/cop/lint/shadowed_exception.rb
Inspecting 1 file
C

Offenses:

lib/rubocop/cop/lint/shadowed_exception.rb:73:9: C: Cyclomatic complexity for contains_multiple_levels_of_exceptions? is too high. [8/6]
        def contains_multiple_levels_of_exceptions?(group)
        ^^^
lib/rubocop/cop/lint/shadowed_exception.rb:73:9: C: Perceived complexity for contains_multiple_levels_of_exceptions? is too high. [9/7]
        def contains_multiple_levels_of_exceptions?(group)
        ^^^
lib/rubocop/cop/lint/shadowed_exception.rb:80:37: C: Avoid using {...} for multi-line blocks.
          group.combination(2).any? { |a, b|
                                    ^

1 file inspected, 3 offenses detected

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).
[Fix #4843] Fix shadow rescued Exceptions of same error code
Note: System dependent error code depends on runtime environment.

@koic koic force-pushed the koic:fix_shadow_rescued_exceptions_of_same_errno branch from b803c62 to ad4f0f2 Oct 7, 2017

Refactor against offenses
Fix the following offenses.

```console
% rubocop lib/rubocop/cop/lint/shadowed_exception.rb
Inspecting 1 file
C

Offenses:

lib/rubocop/cop/lint/shadowed_exception.rb:73:9: C: Cyclomatic
complexity for contains_multiple_levels_of_exceptions? is too
high. [8/6]
        def contains_multiple_levels_of_exceptions?(group)
        ^^^
lib/rubocop/cop/lint/shadowed_exception.rb:73:9: C: Perceived complexity
for contains_multiple_levels_of_exceptions? is too high. [9/7]
        def contains_multiple_levels_of_exceptions?(group)
        ^^^
lib/rubocop/cop/lint/shadowed_exception.rb:80:37: C: Avoid using {...}
for multi-line blocks.
          group.combination(2).any? { |a, b|
                         ^

1 file inspected, 3 offenses detected
```

@koic koic force-pushed the koic:fix_shadow_rescued_exceptions_of_same_errno branch from ad4f0f2 to 1db316c Oct 7, 2017

@Drenmi

Drenmi approved these changes Oct 7, 2017


def compare_exceptions(a, b)
if system_call_error?(a) && system_call_error?(b)
a.const_get(:Errno) != b.const_get(:Errno)

This comment has been minimized.

Copy link
@reitermarkus

reitermarkus Oct 7, 2017

Contributor

I'm not sure this will work.

I have the following:

begin
  something
rescue Errno::EAGAIN, Errno::EWOULDBLOCK, Errno::ECONNABORTED, Errno::EPROTO, Errno::EINTR
  handle_exception
end

Now it will report an offense, because i.e. Errno::EAGAIN::Errno != Errno::ECONNABORTED, or am I missing something?

This comment has been minimized.

Copy link
@koic

koic Oct 8, 2017

Author Member

Thanks for the feedback and reproduce code! This was not enough condition 馃挦
I think that this issue could be solved with the following commit.
5b67c78

@@ -29,6 +29,7 @@
* [#4830](https://github.com/bbatsov/rubocop/issues/4830): Prevent `Lint/BooleanSymbol` from truncating symbol's value in the message when offense is located in the new syntax hash. ([@akhramov][])
* [#4747](https://github.com/bbatsov/rubocop/issues/4747): Fix `Rails/HasManyOrHasOneDependent` cop incorrectly flags `with_options` blocks. ([@koic][])
* [#4836](https://github.com/bbatsov/rubocop/issues/4836): Make `Rails/OutputSafety` aware of safe navigation operator. ([@drenmi][])
* [#4843](https://github.com/bbatsov/rubocop/issues/4843): Fix `Lint/ShadowedException` cop aware of same system error code. ([@koic][])

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 9, 2017

Collaborator

Fix -> Make

@@ -60,6 +60,21 @@
RUBY
end

it 'accepts rescuing contain multiple same error code exceptions' do

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 9, 2017

Collaborator

rescue containing

This comment has been minimized.

Copy link
@koic

koic Oct 9, 2017

Author Member

I fixed typos in a94b6fb.

end
end

def compare_exceptions(a, b)

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 9, 2017

Collaborator

Not the biggest fan of params named a and b, even if here it's relatively clear what they mean.

This comment has been minimized.

Copy link
@koic

koic Oct 9, 2017

Author Member

I changed the variable name in d88837e. Because the line becomes longer, I'm changing the method name a little.


def compare_exceptions(a, b)
if system_call_error?(a) && system_call_error?(b)
a.const_get(:Errno) != b.const_get(:Errno) && a <=> b

This comment has been minimized.

Copy link
@bbatsov

bbatsov Oct 9, 2017

Collaborator

I'd probably add a comment here explaining the need for this.

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

I'd also add something about all of this in the cop's documentation, as it's a pretty interesting special case.

@koic

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

Thanks for the review. I added a explanation for this special case in 3d99f7e. Also added source code comment to this commit.

# # the same error code or different error code depends on environment.
# # This good case is for `Errno::EAGAIN` and `Errno::EWOULDBLOCK` with
# # the same error code.
# # It is a bad case in different error code environment.

This comment has been minimized.

Copy link
@reitermarkus

reitermarkus Oct 9, 2017

Contributor

It shouldn't be a bad case in any environment.

This comment has been minimized.

Copy link
@koic

koic Oct 9, 2017

Author Member

Oops! This is my mistake. Thanks.

@koic koic force-pushed the koic:fix_shadow_rescued_exceptions_of_same_errno branch from 5909920 to 3d99f7e Oct 9, 2017

@bbatsov bbatsov merged commit 18c74f5 into rubocop-hq:master Oct 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

馃憤

@koic koic deleted the koic:fix_shadow_rescued_exceptions_of_same_errno branch Oct 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.