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

Breadcrumbs/leave breadcrumb #511

Merged
merged 4 commits into from
Dec 12, 2018
Merged

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Dec 11, 2018

Goal

Adds the leave_breadcrumb method and associated tests for the Breadcrumbs implementation.

@Cawllec Cawllec requested review from tobyhs, kellymason and a team December 11, 2018 12:24
@Cawllec Cawllec changed the base branch from master to breadcrumbs/base December 11, 2018 12:24
@Cawllec
Copy link
Contributor Author

Cawllec commented Dec 11, 2018

Rubocop has an issue with the length of the Bugsnag module. While this seems arbitrary, and we can probably just bump the rubocop maximum up, any feedback on refactoring would be appreciated.

Copy link
Contributor

@tobyhs tobyhs left a comment

Choose a reason for hiding this comment

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

minor comments, otherwise lgtm

@@ -302,7 +302,7 @@ Metrics/MethodLength:
# Offense count: 1
# Configuration parameters: CountComments.
Metrics/ModuleLength:
Max: 125
Max: 140
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be preferable to add lib/bugsnag.rb to Exclude instead of repeatedly bumping this limit.

lib/bugsnag.rb Outdated
unless breadcrumb.ignore?
# Run callbacks
configuration.before_breadcrumb_callbacks.each do |c|
(c.arity > 0 ? c.call(breadcrumb) : c.call)
Copy link
Contributor

Choose a reason for hiding this comment

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

Outermost parentheses are unnecessary here

# Run callbacks
configuration.before_breadcrumb_callbacks.each do |c|
(c.arity > 0 ? c.call(breadcrumb) : c.call)
break if breadcrumb.ignore?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can return here instead of breaking because it shouldn't proceed further if it is ignored.

@@ -133,4 +133,153 @@ module Kernel
Kernel.send(:remove_const, :REQUIRED)
end
end

describe "#leave_breadcrumb" do
Copy link
Contributor

Choose a reason for hiding this comment

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

.leave_breadcrumb (. instead of # as it is a module/class method on Bugsnag instead of an instance method)

end

it "runs callbacks before leaving" do
Bugsnag.configuration.before_breadcrumb_callbacks << Proc.new { |breadcrumb|
Copy link
Contributor

Choose a reason for hiding this comment

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

I normally see do + end for multi-line blocks instead of curly braces ("Avoid using {...} for multi-line blocks" at https://github.com/rubocop-hq/ruby-style-guide#single-line-blocks )

lib/bugsnag.rb Outdated
break if breadcrumb.ignore?
end

# Validate again incase of callback alteration
Copy link
Contributor

Choose a reason for hiding this comment

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

"in case" (with a space in between)

@Cawllec Cawllec merged commit 7d097a1 into breadcrumbs/base Dec 12, 2018
@Cawllec Cawllec deleted the breadcrumbs/leave-breadcrumb branch December 12, 2018 10:13
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.

2 participants