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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Configuration#use_exception_level_filters option #588

Merged
merged 2 commits into from
May 5, 2017

Conversation

jondeandres
Copy link
Contributor

@jondeandres jondeandres commented Apr 19, 2017

Let the users to set a global use_exception_level_filters option

Usage:

Rollbar.configure do |config|
  config.use_exception_level_filters = true
end

By default the setting is false so we keep the current interface

Fix #587

@rokob
Copy link
Contributor

rokob commented Apr 19, 2017

This seems fine to me. I guess the only question is whether you want this global configuration setting to be overridable on a per-call basis, i.e. if use_exception_level_filters is true in the configuration, based on this code, you cannot add :use_exception_level_filters => false on a particular error call and expect that to work. I think that is okay and conforms to what I would expect by setting the global flag. So I'd say go ahead and merge this, once you fix the tests.

@brianr
Copy link
Member

brianr commented Apr 19, 2017

Hm I think I would expect that:

  • the new flag @jondeandres added is the default
  • the default can be overridden by passing a param in the call

What if we do something like:

  • rename the new config param to use_exception_level_filters_default. Its default will be false to maintain the current behavior.
  • if you don't pass the :use_exception_level_filters param in a call, then the value of the config param is used. (maybe do this by having the param default to nil)
  • if you do pass the :use_exception_level_filters param, then the value you provide is used

@jondeandres jondeandres force-pushed the global-use_exception_level_filters branch from de34c84 to 76b0ff6 Compare April 20, 2017 10:50
Let the users to set a global `use_exception_level_filters_default` option

Usage:

Rollbar.configure do |config|
  config.use_exception_level_filters_default = true
end

By default the setting is `false` so we keep the current interface

Fix #587
@jondeandres jondeandres force-pushed the global-use_exception_level_filters branch from 76b0ff6 to e3a890b Compare April 20, 2017 10:55
@jondeandres
Copy link
Contributor Author

@brianr @rokob I've pushed force here with Brian's suggestions.


expect(Rollbar.last_report[:level]).to be_eql('warning')
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a test when config.use_exception_level_filters_default = true has been set, and when :use_exception_level_filters => false is passed in with the Rollbar.error call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one

@rokob rokob force-pushed the global-use_exception_level_filters branch from b44bdbe to aa96d4f Compare May 5, 2017 00:22
@rokob
Copy link
Contributor

rokob commented May 5, 2017

I squashed those commits.

@rokob rokob merged commit 9ef189d into master May 5, 2017
@waltjones waltjones deleted the global-use_exception_level_filters branch June 27, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants