Add AllowedCompactTypes option for Style/RaiseArgs #8175

pdobb · 2020-06-20T07:10:18Z

Style/RaiseArgs -- Add `AllowedCompactTypes` Option

Style/RaiseArgs:
  AllowedCompactTypes: [] # default

Add custom exception names that should be ignored when
Style/RaiseArgs investigates code with EnforcedStyle: exploded.
This is useful for certain exception types where we really do want an
instance of an exception to be raised. e.g. in Rails:

raise ActiveRecord::RecordInvalid.new(my_model)

Or to raise a custom error type that wraps an object.

To allow the above to pass even while EnforcedStyle: exploded is set
while still not allowing other Exception types:

Style/RaiseArgs:
  AllowedCompactTypes:
    - 'ActiveRecord::RecordInvalid'
    - 'MyCustomWrappedError'

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).
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.
The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

bbatsov · 2020-07-07T16:38:12Z

I understand the reasoning behind the proposed change, but I'm a bit wary of adding an option that affects only one of the styles. Let's see what the others think about the change as well.

bbatsov · 2020-07-07T16:38:47Z

config/default.yml

  EnforcedStyle: exploded
  SupportedStyles:
    - compact # raise Exception.new(msg)
    - exploded # raise Exception, msg
+  AllowedNonExplodedTypes: []


Probably we should stick to the "compact" terminology here, if we decide to go forward with this change.

I can get behind that, thanks. I was trying to convey that it only affects the "Exploded" style. But I also agree with your comment that it's awkward that this option only affects one of the styles.

jonas054

I agree with @bbatsov about the name. Don't see any problems with the parameter only affecting one style.

jonas054 · 2020-07-09T06:59:06Z

spec/rubocop/cop/style/raise_args_spec.rb

+          expect_no_offenses('raise Ex1.new(msg)')
+        end
+
+        it 'skips auto-correction to exploded style' do


I don't think it's necessary to check that auto-correction isn't done when no offenses are found. It's such a general thing, that we only auto-correct found offenses.

jonas054 · 2020-07-09T06:59:35Z

spec/rubocop/cop/style/raise_args_spec.rb

+          expect_no_offenses('raise Ex1.new(arg1, arg2)')
+        end
+
+        it 'skips auto-correction to exploded style' do


Also unnecessary to check, IMO.

jonas054 · 2020-07-09T07:01:35Z

spec/rubocop/cop/style/raise_args_spec.rb

+      let(:cop_config) do
+        {
+          'EnforcedStyle' => 'exploded',
+          'AllowedNonExplodedTypes' => [%w[Ex1 Ex2], ['Ex1']].sample


I don't think we have any other specs where some of the set-up is random. We usually do shared examples and test both (all) cases every time.

My intention was to show that all of the options are equivalent, without wasting the processing time. But, I understand that this isn't common so I'm happy to change this.

pdobb · 2020-07-09T18:50:54Z

@jonas054 Updated as a fixup commit for easy diff verification. Will squash if accepted.

UPDATE: Squashed down to 1 commit.

pirj · 2020-07-10T07:54:10Z

This is useful for certain exception types where we really do want an instance of an exception to be raised. e.g. in Rails:

raise ActiveRecord::RecordInvalid.new(my_model)

Sorry, doesn't it do the same as:

raise ActiveRecord::RecordInvalid, my_model

It would be quite cumbersome to list all the exceptions that accept arguments in .rubocop.yml.

pdobb · 2020-07-10T16:11:52Z

@pirj Yes, I understand / you're right that one can spell out raise ActiveRecord::RecordInvalid, my_model. For my team:

Certain custom exception types with specialized initializers just feel wrong to "explode", while others do not
Our preference is not to have to rely on that ~~3rd~~ 2nd attribute sometimes being a parameter that is passed to the exception's initializer and sometimes just a message. We want to be explicit.

I just used ActiveRecord::RecordInvalid as an example because it is a well known exception type that takes an argument. But basically I'm trying to get enforceability of the "exploded" style while still letting my team tell the story they want to tell with certain custom exception types.

pirj · 2020-07-12T10:03:34Z

Our preference is not to have to rely on that 3rd attribute sometimes being a parameter that is passed to the exception's initializer and sometimes just a message. We want to be explicit.

As far as I recollect, the third argument passed to raise won't be delegated to exception initializer anyway.

So:

# good
raise E
raise E, "message"
raise E, model

# Ruby error!
raise E, model, "message"
# ArgumentError: wrong number of arguments (given 1, expected 2)

And I guess it's not RuboCop's business to deal with raise offences for what Ruby considers to be incorrect code.

Please correct me if I'm mistaken, but this example for compact style is wrong:

      #   raise RuntimeError, arg1, arg2, arg3

pdobb · 2020-07-12T18:02:27Z

As far as I recollect, the third argument passed to raise won't be delegated to exception initializer anyway.

@pirj Yes. I said the wrong thing earlier (now corrected). I meant to say the 2nd argument.

# Using the exploded style:

# good
raise E, "message"

# doesn't make my team happy
raise E, model

# would prefer to add an exception on the exploded style for E so that this is valid:
raise E.new(model)

For example:

# If we use the default exploded style
raise E, model

# ... and then we want to add a message, we can't just do this because it won't work 
# (ruby think `"message"` is the caller / stack trace)
raise E, model, "message"
# so we have to add code churn (change code that isn't directly related to the 
# change we are making)
raise E.new(model), "message"

pirj · 2020-07-13T10:00:19Z

Agree that for

raise MyWrappedError.new(obj), 'message'

there's no other way around.

So we have those cases:

# all good
raise E, "message"

# still good, most probably it's a message
raise E, SOME_FAILURE_TEXT

# ambiguous - message or object? can lead to churn when message is added; confusing
raise E, var_with_failure_text
raise E, method_with_failure_text(error_arg_for_interpolation)

# good - clear what is what
raise E.new(object), "message"
raise E.new(object), SOME_FAILURE_TEXT
raise E.new(object), method_with_failure_text

I wouldn't be so worried about code churn when it comes to readability, and the reader has to figure out if it's a message or an object passed to the exception.

I'm thinking about a more generic solution to this so an exclusion list becomes unnecessary. What are your thoughts on this?

pdobb · 2020-07-13T15:18:30Z

@pirj That sounds great. It does seem basically deterministic. At least enough so that non-deterministic exceptions can be disabled on a case by case basis.

pirj · 2020-07-14T10:10:16Z

Please don't get me wrong, @pdobb , I've started this discussion because even though this will fit your team's style quite well, it may turn out to be used exclusively by your team. This option is a code that needs to be supported and with team style evolution this feature may become orphaned.

This cop needs love and it seems it can be improved - for everyone. Will the cop be able to fit your team style and be beneficial for a wider audience?

I don't mind that addition (FYI: I'm just a bystander here, not affiliated with RuboCop core), and it's approved by the two long-time maintainers. Anyway, wondering how you see this pull request's destiny?

pirj · 2020-09-14T11:14:51Z

@pdobb Ping

pdobb · 2020-09-14T16:22:24Z

@pirj Sorry, life happened. I think you made great points but I don't think I have the time to learn how to implement the dynamic solution right now or any time soon. The change I made in this PR is sufficient for my team's needs still. If it's not accepted I'm OK with just continuing to disable this cop as needed in my projects.

bbatsov · 2020-11-04T15:45:06Z

@pdobb Rebase this and I'll merge it. Seems no one has a better solution or the time to work on one.

```yaml Style/RaiseArgs AllowedCompactTypes: [] # default ``` Add custom exception names that should be ignored when Style/RaiseArgs investigates code with `EnforcedStyle: exploded`. This is useful for certain exception types where we really do want an instance of an exception to be raised. e.g. in Rails: ```ruby raise ActiveRecord::RecordInvalid.new(my_model) ``` Or to raise a custom error type that wraps an object. To allow the above to pass even while `EnforcedStyle: exploded` is set while still not allowing other Exception types: ```yaml Style/RaiseArgs AllowedCompactTypes: - 'ActiveRecord::RecordInvalid' - 'MyCustomWrappedError' ```

pdobb · 2020-11-04T18:24:56Z

@bbatsov all set, thanks

bbatsov · 2020-11-04T18:52:51Z

Thanks!

pdobb force-pushed the pd/add_allowed_non_exploded_types_option_for_style_raise_args branch 2 times, most recently from e18c89a to 192e4c5 Compare June 20, 2020 08:09

pdobb force-pushed the pd/add_allowed_non_exploded_types_option_for_style_raise_args branch 2 times, most recently from f6c8cb2 to ed58764 Compare June 27, 2020 21:48

bbatsov reviewed Jul 7, 2020

View reviewed changes

bbatsov requested review from jonas054, pirj and koic July 7, 2020 16:39

jonas054 requested changes Jul 9, 2020

View reviewed changes

pdobb force-pushed the pd/add_allowed_non_exploded_types_option_for_style_raise_args branch 3 times, most recently from eea16dc to 3495b43 Compare July 9, 2020 18:49

pdobb changed the title ~~Add AllowedNonExplodedTypes option for Style/RaiseArgs~~ Add AllowedCompactTypes option for Style/RaiseArgs Jul 9, 2020

bbatsov approved these changes Jul 9, 2020

View reviewed changes

jonas054 approved these changes Jul 10, 2020

View reviewed changes

pdobb force-pushed the pd/add_allowed_non_exploded_types_option_for_style_raise_args branch from 3495b43 to 9568b98 Compare July 10, 2020 17:56

pdobb force-pushed the pd/add_allowed_non_exploded_types_option_for_style_raise_args branch from 9568b98 to b8d437f Compare November 4, 2020 18:07

bbatsov merged commit e46eeb6 into rubocop:master Nov 4, 2020

This was referenced Feb 23, 2021

Suggestion: Prefer to _not_ use two argument version of raise rubocop/ruby-style-guide#747

Closed

Drop rule prohibiting passing exception instances to fail rubocop/ruby-style-guide#395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AllowedCompactTypes option for Style/RaiseArgs #8175

Add AllowedCompactTypes option for Style/RaiseArgs #8175

pdobb commented Jun 20, 2020 •

edited

Loading

bbatsov commented Jul 7, 2020

bbatsov Jul 7, 2020

pdobb Jul 7, 2020

jonas054 left a comment

jonas054 Jul 9, 2020

jonas054 Jul 9, 2020

jonas054 Jul 9, 2020

pdobb Jul 9, 2020 •

edited

Loading

pdobb commented Jul 9, 2020 •

edited

Loading

pirj commented Jul 10, 2020

pdobb commented Jul 10, 2020 •

edited

Loading

pirj commented Jul 12, 2020

pdobb commented Jul 12, 2020 •

edited

Loading

pirj commented Jul 13, 2020 •

edited

Loading

pdobb commented Jul 13, 2020

pirj commented Jul 14, 2020

pirj commented Sep 14, 2020

pdobb commented Sep 14, 2020

bbatsov commented Nov 4, 2020

pdobb commented Nov 4, 2020

bbatsov commented Nov 4, 2020

Add AllowedCompactTypes option for Style/RaiseArgs #8175

Add AllowedCompactTypes option for Style/RaiseArgs #8175

Conversation

pdobb commented Jun 20, 2020 • edited Loading

Style/RaiseArgs -- Add AllowedCompactTypes Option

bbatsov commented Jul 7, 2020

bbatsov Jul 7, 2020

Choose a reason for hiding this comment

pdobb Jul 7, 2020

Choose a reason for hiding this comment

jonas054 left a comment

Choose a reason for hiding this comment

jonas054 Jul 9, 2020

Choose a reason for hiding this comment

jonas054 Jul 9, 2020

Choose a reason for hiding this comment

jonas054 Jul 9, 2020

Choose a reason for hiding this comment

pdobb Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

pdobb commented Jul 9, 2020 • edited Loading

pirj commented Jul 10, 2020

pdobb commented Jul 10, 2020 • edited Loading

pirj commented Jul 12, 2020

pdobb commented Jul 12, 2020 • edited Loading

pirj commented Jul 13, 2020 • edited Loading

pdobb commented Jul 13, 2020

pirj commented Jul 14, 2020

pirj commented Sep 14, 2020

pdobb commented Sep 14, 2020

bbatsov commented Nov 4, 2020

pdobb commented Nov 4, 2020

bbatsov commented Nov 4, 2020

pdobb commented Jun 20, 2020 •

edited

Loading

Style/RaiseArgs -- Add `AllowedCompactTypes` Option

pdobb Jul 9, 2020 •

edited

Loading

pdobb commented Jul 9, 2020 •

edited

Loading

pdobb commented Jul 10, 2020 •

edited

Loading

pdobb commented Jul 12, 2020 •

edited

Loading

pirj commented Jul 13, 2020 •

edited

Loading