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

An option for Lint/HandleExceptions to accept comments #7052

Closed
dduugg opened this issue May 15, 2019 · 7 comments
Closed

An option for Lint/HandleExceptions to accept comments #7052

dduugg opened this issue May 15, 2019 · 7 comments
Labels
feature request

Comments

@dduugg
Copy link
Contributor

dduugg commented May 15, 2019 •
edited

Is your feature request related to a problem? Please describe.

I find that Lint/HandleExceptions is often bypassed with a nil in the rescue block. This is not helpful, whereas comments would be. However, a rescue block consisting of only a comment will still trigger a Lint/HandleExceptions offense.

Describe the solution you'd like

I would like an option to allow comments within a rescue block to avoid Lint/HandleExceptions. Perhaps AllowComments: true.

Describe alternatives you've considered

  • Instead of a config option, allow comments in all cases
  • Disable the cop
  • Also ban rescue blocks consisting of nil with no add'l context
  • Continue with status quo

Additional context

n/a
tejasbubane added a commit to tejasbubane/rubocop that referenced this issue May 20, 2019
@tejasbubane
Add AllowComments option to Lint/HandleExceptions
ae609b8 
Issue: rubocop#7052

To allow empty rescue blocks with comments
@tejasbubane tejasbubane mentioned this issue May 20, 2019
Merged
8 tasks
@tejasbubane
Copy link
Contributor

There are two parts to this:

  1. Add config option AllowComments - done in Add AllowComments option to Lint/HandleExceptions #7060
  2. Do not allow rescue blocks consisting of nil (especially single line rescue) - I will do this once above is merged to avoid conflicts.

bbatsov pushed a commit that referenced this issue May 21, 2019
@tejasbubane @bbatsov
Add AllowComments option to Lint/HandleExceptions
2aca931 
Issue: #7052

To allow empty rescue blocks with comments
koic added a commit to koic/rubocop that referenced this issue May 21, 2019
@koic
Fix a CI error
6a4f586 
Follow up rubocop@8a7ee84.

This PR fixes the following CI errors.

```console
bundle exec rake

(snip)

  1) RuboCop Project changelog entry after version 0.14.0 has a link to
  the contributors at the end
     Failure/Error: expect(entries).to all(match(/\(\[@\S+\]\[\](?:,
  \[@\S+\]\[\])*\)$/))

       expected ["*
       [rubocop#6649](rubocop#6649):
       `Layout/AlignHash` supports list of opt...552): `RaiseArgs`
       allows exception constructor calls with more than one 1
       argument. ([@bbatsov][])"] to all match /\(\[@\S+\]\[\](?:,
       \[@\S+\]\[\])*\)$/

          object at index 3 failed to match:
             expected "*
             [rubocop#7052](rubocop#7052):
             Add `AllowComments` option to `
             Lint/HandleExceptions`. ([@tejasbubane][]) " to match
             /\(\[@\S+\]\[\](?:, \[@\S+\]\[\])*\)$/
     # ./spec/project_spec.rb:128:in `block (5 levels) in <top (required)>'

(snip)

  2) RuboCop Project changelog entry body ends with a punctuation
     Failure/Error: expect(bodies).to all(match(/[\.\!]$/))

       expected ["`` supports list of options.", "Add `` config option
       to ``.", "Add `` to ``.", "Add `` option to ``...nclosed in
       braces are not noticed.", "Received malformed format string
       ArgumentError from rubocop."] to all match /[\.\!]$/

          object at index 3 failed to match:
             expected "Add `` option to ``. ([@tejasbubane][]) " to
             match /[\.\!]$/
     # ./spec/project_spec.rb:187:in `block (5 levels) in <top (required)>'
```

https://circleci.com/gh/rubocop-hq/rubocop/55378
@rmm5t
Copy link
Contributor

rmm5t commented May 21, 2019

I like this, and I think it should be the new default.

@stale
Copy link

stale bot commented Aug 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label Aug 19, 2019
tejasbubane added a commit to tejasbubane/rubocop that referenced this issue Aug 20, 2019
@tejasbubane
[Fix rubocop#7052] Make Lint/HandleExceptions register offense for …
8e686ba 
…rescue nil
@tejasbubane tejasbubane mentioned this issue Aug 20, 2019
Closed
8 tasks
@tejasbubane
Copy link
Contributor

Fixed in #7297

@stale stale bot removed the stale Issues that haven't been active in a while label Aug 20, 2019
@stale
Copy link

stale bot commented Feb 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label Feb 16, 2020
@sergeyk
Copy link

sergeyk commented Mar 18, 2020

This issue should be closed, as it was fixed

@stale stale bot removed the stale Issues that haven't been active in a while label Mar 18, 2020
@koic koic closed this as completed Mar 18, 2020
koic added a commit to koic/rubocop that referenced this issue Mar 18, 2020
@koic
Change AllowComments of Lint/SuppressedException to true by default
b23822d 
## Summary

Follow rubocop#7052 (comment).

This PR changes `AllowComments` option of `Lint/SuppressedException` to
true by default.

As is used in RuboCop repo itself, it is a common practice to use
source code comments to explain when exception handling is not performed.

## Related Information

I think that `Suppressing Exceptions` rule can be refined to a suitable example.
https://github.com/rubocop-hq/ruby-style-guide#dont-hide-exceptions
@koic koic mentioned this issue Mar 18, 2020
Merged
8 tasks
@koic
Copy link
Member

koic commented Mar 18, 2020

I agree with @rmm5t. I opened PR #7805.

bbatsov pushed a commit that referenced this issue Mar 19, 2020
@koic @bbatsov
Change AllowComments of Lint/SuppressedException to true by default
84d6c57 
## Summary

Follow #7052 (comment).

This PR changes `AllowComments` option of `Lint/SuppressedException` to
true by default.

As is used in RuboCop repo itself, it is a common practice to use
source code comments to explain when exception handling is not performed.

## Related Information

I think that `Suppressing Exceptions` rule can be refined to a suitable example.
https://github.com/rubocop-hq/ruby-style-guide#dont-hide-exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #7052] Make Lint/HandleExceptions register offense for rescue nil tejasbubane/rubocop
6 participants
@rmm5t @sergeyk @koic @bbatsov @dduugg @tejasbubane