Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(iOS): assign -> copy for numeric types #61

Merged
merged 2 commits into from
Jun 4, 2019
Merged

fix(iOS): assign -> copy for numeric types #61

merged 2 commits into from
Jun 4, 2019

Conversation

colinking
Copy link
Contributor

@colinking colinking commented Jun 3, 2019

Resolves an issue in iOS clients for required numeric values where the incorrect value was supplied to analytics-ios.

@colinking colinking marked this pull request as ready for review June 4, 2019 00:51
Copy link

@f2prateek f2prateek left a comment

Choose a reason for hiding this comment

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

LGTM! Should we add a test case to prevent a future regression?

Copy link

@fathyb fathyb left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -564,9 +565,35 @@ class AnalyticsObjectiveCWrapperRenderer extends ObjectiveCRenderer {
})
}

/**
* Always used the boxed types, because of a potential upstream bu with nullable values
Copy link

Choose a reason for hiding this comment

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

upstream bug*? 😄

@colinking
Copy link
Contributor Author

@f2prateek we're adding a harness for these kind of run-time tests in v7: #59

@colinking colinking merged commit 7a2ec99 into master Jun 4, 2019
@colinking colinking deleted the colin/copy branch June 4, 2019 17:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants