Skip to content

Conversation

@rxlabz
Copy link
Contributor

@rxlabz rxlabz commented Oct 29, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

add a contexts property to Scope

💡 Motivation and Context

https://develop.sentry.dev/sdk/unified-api/

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

@rxlabz rxlabz marked this pull request as ready for review October 29, 2020 19:44
@marandaneto
Copy link
Contributor

this PR is not addressing the SentryClient changes:
which is merging the Event Contexts and the Scope Contexts
https://github.com/getsentry/sentry-java/blob/5ebd49cbfd17eb66175e9247db6d27e4df84a56b/sentry/src/main/java/io/sentry/SentryClient.java#L317-L321

@rxlabz
Copy link
Contributor Author

rxlabz commented Oct 29, 2020

this PR is not addressing the SentryClient changes:
which is merging the Event Contexts and the Scope Contexts

@marandaneto is this incorrect or incomplete?

contexts.clone().forEach((key, value) {
if (!event.contexts.containsKey(key) && value != null) {
event.contexts[key] = value;
}
});

@marandaneto
Copy link
Contributor

this PR is not addressing the SentryClient changes:
which is merging the Event Contexts and the Scope Contexts

@marandaneto is this incorrect or incomplete?

contexts.clone().forEach((key, value) {
if (!event.contexts.containsKey(key) && value != null) {
event.contexts[key] = value;
}
});

more to incomplete than incorrect, I've made a few new comments

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

@rxlabz rxlabz merged commit c80af60 into main Oct 30, 2020
@rxlabz rxlabz deleted the feat/scope-contexts branch October 30, 2020 14:16
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