-
Notifications
You must be signed in to change notification settings - Fork 93
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
createTracker
now returns non-optional TrackerController
#848
createTracker
now returns non-optional TrackerController
#848
Conversation
Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign. Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. |
Sources/Snowplow/Snowplow.swift
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the meat of the PR 🍖
Signed! 👇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution @Kymer! As you said in the issue, making the TrackerController
was an oversight during the Swift migration of the codebase. We'll make sure that this gets into the v6 release!
Just one small thing – could you please rebase your changes on top of the release/6.0.0
branch? There seem to be some conflicts.
46a7d60
to
7243ee8
Compare
Done! |
7243ee8
to
d37a1d6
Compare
d37a1d6
to
46838d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks again for the contribution!
This PR fixes #847. All tests compile and run fine: everything was green except for the
IntegrationTests
. These fail because I don't have a local service running. I assume I need to set-up Snowplow Micro to get this working?Anyway, this is a very small change. Having the tests compile gave me enough confidence to open this PR.