filter with raw exception instance #241

kuboon · 2017-07-21T15:25:08Z

README shows following example to ignore StandardError

Airbrake.add_filter do |notice|
  if notice[:errors].any? { |error| error[:type] == 'StandardError' }
    notice.ignore!
  end
end

But we rarely raise StandardError itself.
Instead, we raise the descendants of StandardError.

Current implementation converts the exception to a string value before filtering.
It looses class hierarchy.

This patch allows filter to use exception instance to do like this:

Airbrake.add_filter do |notice|
  notice.ignore! if notice.exception.kind_of? StandardError
end

kyrylo

This is a cool change 👍
I've been thinking about doing this, but there was no user demand, so I thought it wasn't needed.

It would be nice to add a test for the change, then I can merge the PR. Thanks!

kyrylo · 2017-07-22T00:28:59Z

README.md

-  if notice[:errors].any? { |error| error[:type] == 'StandardError' }
-    notice.ignore!
-  end
+  notice.ignore! if notice.exception.kind_of? StandardError


Please use brackets around .kind_of?(StandardError). We prefer this style in this project.

kyrylo · 2017-07-22T00:29:49Z

lib/airbrake-ruby/notice.rb

@@ -62,7 +62,7 @@ class Notice
    # @since v1.7.0
    # @return [Hash{Symbol=>Object}] the hash with arbitrary objects to be used
    #   in filters
-    attr_reader :stash
+    attr_reader :stash, :exception


We should probably move this on a separate line because the comment above describes :stash.

kuboon · 2017-07-24T08:42:56Z

Failed on Rubocop. How can I avoid it?

We don't reuse them, so there's no need for them to be separate methods. Besides, this will decrease class length, so we don't introduce any Rubocop offences (#241)

kyrylo · 2017-07-24T13:41:56Z

README.md

-  if notice[:errors].any? { |error| error[:type] == 'StandardError' }
-    notice.ignore!
-  end
+  notice.ignore! if notice.exception.kind_of?(StandardError)


Rubocop wants is_a? instead of kind_of?. They are aliases, so let's use is_a?.

kyrylo · 2017-07-24T13:43:14Z

spec/notifier_spec.rb

@@ -514,6 +514,20 @@ def to_json(*)
      @airbrake.notify_sync(ex)
      expect(a_request(:post, endpoint)).to have_been_made.once
    end
+
+    it "ignores descendant classes" do
+      class AirbrakeTestSubError < AirbrakeTestError; end


AirbrakeTestSubError can be a local variable:

descendant = Class.new(AirbrakeTestError)

It is preferred, so we don't pollute namespaces with constants.

kyrylo · 2017-07-25T00:45:07Z

To fix the Metrics/ClassLength, please rebase on master. I made some refactoring, which should help with this issue.

vmihailenco · 2017-07-25T05:29:43Z

lib/airbrake-ruby/notice.rb

@@ -64,6 +64,10 @@ class Notice
    #   in filters
    attr_reader :stash

+    ##
+    # @return [Exception] raw exception
+    attr_reader :exception


what about putting this into @stash.exception?

I don't see why not. @kuboon, that will probably the preferred solution. Let me know what you think.

no problem. I did not understand what is stash for.

use is_a? instead of kind_of?

kuboon · 2017-07-26T05:15:36Z

I can't see the detail of Rubocop fail.

kyrylo · 2017-07-26T05:16:39Z

You can run Rubocop locally.

Offenses:

lib/airbrake-ruby/notice.rb:77:16: C: Layout/SpaceInsideHashLiteralBraces: Space inside { missing. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
      @stash = {exception: exception}
               ^
lib/airbrake-ruby/notice.rb:77:37: C: Layout/SpaceInsideHashLiteralBraces: Space inside } missing. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
      @stash = {exception: exception}
                                    ^

kyrylo · 2017-07-26T09:15:53Z

Great work, thanks 👍

filter with raw exception instance

6cc5a71

kyrylo suggested changes Jul 22, 2017

View reviewed changes

kuboon added 2 commits July 24, 2017 16:58

add spec for notice.exception

74c1233

we need brackets

4f2376a

kyrylo added a commit that referenced this pull request Jul 25, 2017

notice: inline #[]= validation methods

2b3eac7

We don't reuse them, so there's no need for them to be separate methods. Besides, this will decrease class length, so we don't introduce any Rubocop offences (#241)

kyrylo mentioned this pull request Jul 25, 2017

notice: inline #[]= validation methods #242

Merged

kyrylo suggested changes Jul 25, 2017

View reviewed changes

vmihailenco reviewed Jul 25, 2017

View reviewed changes

kuboon added 2 commits July 25, 2017 17:18

Merge remote-tracking branch 'base/master'

216589d

exception attr -> stash[:exception]

1ca2b78

use is_a? instead of kind_of?

add spaces inside brackets

3bcfc09

kyrylo approved these changes Jul 26, 2017

View reviewed changes

kyrylo merged commit 68599e8 into airbrake:master Jul 26, 2017

kuboon deleted the patch-1 branch July 26, 2017 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filter with raw exception instance #241

filter with raw exception instance #241

kuboon commented Jul 21, 2017 •

edited

Loading

kyrylo left a comment

kyrylo Jul 22, 2017

kyrylo Jul 22, 2017

kuboon commented Jul 24, 2017

kyrylo Jul 24, 2017

kyrylo Jul 24, 2017

kyrylo commented Jul 25, 2017

vmihailenco Jul 25, 2017

kyrylo Jul 25, 2017

kuboon Jul 25, 2017

kuboon commented Jul 26, 2017

kyrylo commented Jul 26, 2017

kyrylo commented Jul 26, 2017

filter with raw exception instance #241

filter with raw exception instance #241

Conversation

kuboon commented Jul 21, 2017 • edited Loading

kyrylo left a comment

Choose a reason for hiding this comment

kyrylo Jul 22, 2017

Choose a reason for hiding this comment

kyrylo Jul 22, 2017

Choose a reason for hiding this comment

kuboon commented Jul 24, 2017

kyrylo Jul 24, 2017

Choose a reason for hiding this comment

kyrylo Jul 24, 2017

Choose a reason for hiding this comment

kyrylo commented Jul 25, 2017

vmihailenco Jul 25, 2017

Choose a reason for hiding this comment

kyrylo Jul 25, 2017

Choose a reason for hiding this comment

kuboon Jul 25, 2017

Choose a reason for hiding this comment

kuboon commented Jul 26, 2017

kyrylo commented Jul 26, 2017

kyrylo commented Jul 26, 2017

kuboon commented Jul 21, 2017 •

edited

Loading