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

Upgrade to use swift 4.2 #6

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

mtjoe
Copy link
Contributor

@mtjoe mtjoe commented Apr 2, 2019

This PR contains the following changes:

  • Upgrade MMPixelSDK to use Swift 4.2
  • Upgrade MMPixelExampleApp to use Swift 4.2
  • Fix Swift errors
  • Fix failing tests

Upon upgrading a Swift 4.2, there was a few errors:

  1. In MMPixelSDK/MMPixelConfig.swift, line 35 & 37:
  • Overlapping accesses to 'name', but modification requires exclusive access; consider copying to a local variable
  • Fixed by changing the 5th argument of sysctl to nil. This argument (newval) is not required to get the hardware name of the device.
  1. In MMPixelSDK/MMPixelSDK.swift, line 107:
  • Initializer for conditional binding must have Optional type, not 'UUID'
  • Fixed by removing the conditional binding.
  1. Tests testGetAddlParamsString and testGetAddlParamsStringWithHashedEmail failing
  • Caused by the expected params string to have the wrong ordering to the addlParams. This is because Swift dictionaries do not have ordering. Therefore, upon creation of the addlParams dictionary, the ordering of the addlParams was changed.
  • Fixed by changing testing strategy to check whether each param is contained inside the generated string in different assertion statements.

@fsargent
Copy link

fsargent commented Apr 2, 2019

This is great. I think you should be able to get the tests to pass by upgrading the .travis.yml with xcode10.2
https://docs.travis-ci.com/user/reference/osx/#xcode-version

Copy link
Contributor

@FodT FodT left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
Right now, the travis build is failing. Can you update .travis.yml to use xcode10.2 ?

@mtjoe
Copy link
Contributor Author

mtjoe commented Apr 2, 2019

Travic CI is passing now 👍

Copy link
Contributor

@FodT FodT left a comment

Choose a reason for hiding this comment

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

Thank you!

@FodT FodT merged commit f735df3 into MediaMath:master Apr 3, 2019
@mtjoe mtjoe deleted the upgrade/swift-4.2 branch April 3, 2019 23:27
# 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