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

mergeAttributes: deepcopy RHS Map if LHS isn't Map #23

Merged
merged 2 commits into from
Oct 9, 2018
Merged

mergeAttributes: deepcopy RHS Map if LHS isn't Map #23

merged 2 commits into from
Oct 9, 2018

Conversation

dotdoom
Copy link
Contributor

@dotdoom dotdoom commented Oct 8, 2018

No description provided.

@dotdoom dotdoom changed the title mergeAttributes: deepcopy LHS Map if RHS isn't Map mergeAttributes: deepcopy RHS Map if LHS isn't Map Oct 8, 2018
@dotdoom
Copy link
Contributor Author

dotdoom commented Oct 8, 2018

The problem that this PR solves is that a Map present in environmentAttributes gets exposed when initially merged into data. It is then mutated by the following mergeAttributes of event into data.


Looks like Travis build fails because it can't resolve some dependencies with Dart 2. I tested this PR by running echo test/*.dart | xargs -n1 dart.

@yjbanov
Copy link
Contributor

yjbanov commented Oct 8, 2018

I like this change! Thank you for the fix. I'll have a look at Travis asap (currently AFK).

@yjbanov
Copy link
Contributor

yjbanov commented Oct 8, 2018

Travis is unhappy because the version constraint for mockito prevents it from picking up the latest version of the Dart SDK. Changing it to mockito: ">=2.0.0 <4.0.0" should fix it 🤞

@dotdoom
Copy link
Contributor Author

dotdoom commented Oct 9, 2018

Sent #24 for that, and rebased this PR, too.

@yjbanov
Copy link
Contributor

yjbanov commented Oct 9, 2018

LGTM

@yjbanov yjbanov merged commit 044e4c1 into getsentry:master Oct 9, 2018
@yjbanov
Copy link
Contributor

yjbanov commented Oct 9, 2018

This is now published in version 2.1.1: https://pub.dartlang.org/packages/sentry

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