Breadcrumbs: Configuration updates #507
Conversation
|
This is a first iteration, without tests as of yet, so any reviews should be very quick. |
Cawllec
changed the base branch from
breadcrumbs/base
to
breadcrumbs/breadcrumb-validator
lib/bugsnag/configuration.rb
Outdated
|
|
||
| # Store max_breadcrumbs here instead of outputting breadcrumbs.max_items | ||
| # to avoid infinite recursion when creating breadcrumb buffer | ||
| @max_breadcrumbs = DEFAULT_MAX_BREADCRUMBS.clone |
There was a problem hiding this comment.
DEFAULT_MAX_BREADCRUMBS is an integer, so you shouldn't need to clone it
| @@ -36,6 +37,9 @@ class Configuration | |||
| attr_accessor :auto_capture_sessions | |||
| attr_accessor :track_sessions | |||
| attr_accessor :session_endpoint | |||
| attr_accessor :automatic_breadcrumb_types | |||
|
|
|||
| attr_reader :max_breadcrumbs | |||
There was a problem hiding this comment.
If you're starting to adopt yardoc, it would be good to add documentation/comments to new methods that you add/create.
There was a problem hiding this comment.
To keep things consistent, I was going to add Yardoc comments to new files, and keep existing files as they are, and raise a ticket to Yardoc the whole repo.
There was a problem hiding this comment.
For the sake of these actually getting written, i've added the yardoc comments now and will continue to in all the other changes.
Cawllec
changed the title
lib/bugsnag/configuration.rb
Outdated
| @@ -68,6 +75,12 @@ def initialize | |||
| self.notify_release_stages = nil | |||
| self.auto_capture_sessions = false | |||
| self.session_endpoint = DEFAULT_SESSION_ENDPOINT | |||
| # All valid breadcrumb types should be allowable initially | |||
| self.automatic_breadcrumb_types = Bugsnag::Breadcrumbs::VALID_BREADCRUMB_TYPES.clone | |||
There was a problem hiding this comment.
I think you want dup here. If this isnt already frozen - we should freeze VALID_BREADCRUMB_TYPES
There was a problem hiding this comment.
Yes - and I'm adding the extra test to verify it is mutable.
lib/bugsnag/configuration.rb
Outdated
| ## | ||
| # Stores callbacks that will be run before a breadcrumb is left | ||
| def before_breadcrumb_callbacks | ||
| @before_breadcrumb_callbacks ||= [] |
There was a problem hiding this comment.
It seems generally safer to initialize this in the constructor rather than lazily.
spec/configuration_spec.rb
Outdated
|
|
||
| expect(buffer.to_a).to eq([1]) | ||
| expect(second_buffer.to_a).to eq([2]) | ||
| expect(buffer).to_not eq(second_buffer) |
There was a problem hiding this comment.
You don't need this expect
| end | ||
|
|
||
| describe "#automatic_breadcrumb_types" do | ||
| it "defaults to Bugsnag::Breadcrumbs::VALID_BREADCRUMB_TYPES" do |
There was a problem hiding this comment.
Having some tests around being able to change this would be good
spec/configuration_spec.rb
Outdated
| second_array = nil | ||
| Thread.new { second_array = subject.before_breadcrumb_callbacks; second_array << 2}.join | ||
|
|
||
| expect(first_array).to eql(second_array) |
There was a problem hiding this comment.
I'd drop this expectation
…w automatic_breadcrumb_types editting
lib/bugsnag/configuration.rb
Outdated
| @@ -37,6 +39,18 @@ class Configuration | |||
| attr_accessor :track_sessions | |||
| attr_accessor :session_endpoint | |||
|
|
|||
| ## | |||
| # @return [Array] array of strings indicating allowable automatic breadcrumb types | |||
There was a problem hiding this comment.
Should do [Array] and similar for others
Goal
Update the Configuration class in order to support breadcrumbs.