Breadcrumbs: Add Breadcrumb class #503
Conversation
| self.type = type | ||
| self.meta_data = meta_data | ||
| @auto = auto | ||
| @timestamp = Time.now.utc.strftime('%Y-%m-%dT%H:%M:%S') |
There was a problem hiding this comment.
Should we store as a time and convert to string later?
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| expect(breadcrumb.meta_data).to eq({:a => 1, :b => 2}) | ||
| expect(breadcrumb.auto).to eq(:manual) | ||
|
|
||
| expect(breadcrumb.timestamp).to_not be_nil |
There was a problem hiding this comment.
There should be more constraint on this
| attr_reader :auto | ||
| attr_reader :timestamp | ||
|
|
||
| def initialize(message, type, meta_data, auto) |
There was a problem hiding this comment.
If we're starting to adopt yardoc in bugsnag-ruby, there should be @param, @return, etc. comments (see https://www.rubydoc.info/gems/yard/file/docs/GettingStarted.md , but don't follow that first snippet which actually RDoc instead). Also, #initialize should probably have an # @api private as I don't think end users will need to create a Breadcrumb.
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| describe Bugsnag::Breadcrumbs::Breadcrumb do | ||
| describe "initialize" do | ||
| it "should assign arguments correctly" do | ||
| breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new("my message", "test type", {:a => 1, :b => 2}, :manual) |
There was a problem hiding this comment.
Yes, it will be a boolean inside the class, but not the leave_breadcrumbs method. I'll update that.
| self.type = type | ||
| self.meta_data = meta_data | ||
| @auto = auto | ||
| @timestamp = Time.now.utc.strftime('%Y-%m-%dT%H:%M:%S') |
There was a problem hiding this comment.
This string should probably end in Z to make it conform to a ISO 8601 time
|
|
||
| def to_h | ||
| { | ||
| :message => message, |
There was a problem hiding this comment.
I think the error reporting API takes a name instead of message (from https://bugsnagerrorreportingapi.docs.apiary.io/#reference/0/notify/send-error-reports )
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| require 'bugsnag/breadcrumbs/breadcrumb' | ||
|
|
||
| describe Bugsnag::Breadcrumbs::Breadcrumb do | ||
| describe "initialize" do |
There was a problem hiding this comment.
I prefer describe "#initialize" (with the #) so that the failure message is Bugsnag::Breadcrumbs::Breadcrumb#initialize should assign arguments correctly (similarly for the #to_h describe block)
| @@ -0,0 +1,57 @@ | |||
| module Bugsnag::Breadcrumbs | |||
| class Breadcrumb | |||
| attr_accessor :message | |||
There was a problem hiding this comment.
I know your original design doc had this as message, but it might be best to rename this attribute and the parameter of #initialize to name at this point to be consistent.
| attr_reader :auto | ||
| attr_reader :timestamp | ||
|
|
||
| def initialize(message, type, meta_data, auto) |
There was a problem hiding this comment.
If we're starting to adopt yardoc in bugsnag-ruby, there should be @param, @return, etc. comments (see https://www.rubydoc.info/gems/yard/file/docs/GettingStarted.md , but don't follow that first snippet which actually RDoc instead). Also, #initialize should probably have an # @api private as I don't think end users will need to create a Breadcrumb.
| :name => @message, | ||
| :type => @type, | ||
| :metaData => @meta_data, | ||
| :timestamp => @timestamp.utc.strftime('%Y-%m-%dT%H:%M:%SZ') |
There was a problem hiding this comment.
As mentioned in #503 (comment) you can use the iso8601 method, but you'll probably need to require 'time' in this file for that to work.
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| expect(breadcrumb.auto).to eq(true) | ||
| end | ||
|
|
||
| it "if false if auto argument is anything else" do |
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| it "is stored as a timestamp" do | ||
| breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new(nil, nil, nil, nil) | ||
|
|
||
| expect(breadcrumb.timestamp).to be_within(0.5.second).of Time.now |
There was a problem hiding this comment.
Avoid the second method unless you're willing to bring in activesupport as an explicit development dependency
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| end | ||
| end | ||
|
|
||
| describe "#ignore" do |
There was a problem hiding this comment.
This should be #ignore? (to match the actual method name called by all examples under this describe block)
| # @return [Hash, nil] metadata hash containing strings, numbers, or booleans, or nil | ||
| attr_accessor :meta_data | ||
|
|
||
| # @return [Symbol] set to `:auto` if the breadcrumb was automatically generated |
| @auto = auto == :auto | ||
|
|
||
| # Store it as a timestamp for now | ||
| @timestamp = Time.now |
There was a problem hiding this comment.
We convert to utc before outputting it in the hash
There was a problem hiding this comment.
Do it here so you dont have different timezones in the library. Its a best practice to convert to UTC immediately.
|
|
||
| describe "#type" do | ||
| it "is assigned in #initialize" do | ||
| breadcrumb = Bugsnag::Breadcrumbs::Breadcrumb.new(nil, "test type", nil, nil) |
There was a problem hiding this comment.
its weird that you can set this to an invalid type to me.
There was a problem hiding this comment.
As this may be overwritten by callbacks anyway, it seems better to check this in a single place (the validator) than in the breadcrumb.
There was a problem hiding this comment.
in isolation i would have the setter validate. Its complicated by the metadata validation where that isnt possible however.
spec/breadcrumbs/breadcrumb_spec.rb
Outdated
| :a => 1, | ||
| :b => 2 | ||
| }, | ||
| :timestamp => match(timestamp_regex) |
There was a problem hiding this comment.
Here you should expect it to match timestamp.iso8601 rather than just regexing imo
Adds Bugsnag::Breadcrumbs::Breadcrumb class and tests in order to support Ruby Breadcrumbs feature.