Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

execute beforeBreadcrumb also for scope #160

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

marandaneto
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

execute beforeBreadcrumb also for scope

💡 Motivation and Context

beforeBreadcrumb was not being executed when breadcrumbs were added directly to the scope

💚 How did you test it?

unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

refactor it, we have a design flaw here, I don't like the solution, but we need to have it done till tomorrow.

if (executeBeforeBreadcrumb && beforeBreadcrumbCallback != null) {
try {
breadcrumb =
beforeBreadcrumbCallback.execute(breadcrumb, null); // TODO: whats about hint here?
Copy link
Member

Choose a reason for hiding this comment

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

We should take hint also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bruno-garcia how? if the user has called scope.addBreadcrumb(...) where do we get it from?

Copy link
Member

Choose a reason for hiding this comment

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

We'd need to add an overload that takes hint.

@codecov-io
Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #160 into master will increase coverage by 0.3%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #160     +/-   ##
===========================================
+ Coverage     57.02%   57.33%   +0.3%     
- Complexity      516      521      +5     
===========================================
  Files            72       72             
  Lines          2497     2517     +20     
  Branches        219      221      +2     
===========================================
+ Hits           1424     1443     +19     
  Misses          965      965             
- Partials        108      109      +1
Impacted Files Coverage Δ Complexity Δ
sentry-core/src/main/java/io/sentry/core/Hub.java 66.21% <66.66%> (ø) 38 <0> (ø) ⬇️
...entry-core/src/main/java/io/sentry/core/Scope.java 85.54% <95.23%> (+3%) 27 <6> (+5) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1a514b...eff9bec. Read the comment docs.

@bruno-garcia bruno-garcia merged commit 29f5247 into master Nov 22, 2019
@bruno-garcia bruno-garcia deleted the bug/beforebreadcrumb_scope branch November 22, 2019 17:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants