matomo-org / matomo-sdk-ios

Matomo iOS, tvOS and macOS SDK: a Matomo tracker written in Swift
MIT License
388 stars 164 forks source link

Adds support for Carthage #173

Closed elitalon closed 7 years ago

elitalon commented 7 years ago

This attempts to fix #74 by solving a few build errors that happen when building with carthage --no-skip-current.

Theoretically, that should allow projects managed with Carthage to use the framework.

brototyp commented 7 years ago

Hi @elitalon, thank you very much for your effort! I am not used to use Carthage. Is there any way we can add a Carthage-Build check to the travis file?

elitalon commented 7 years ago

@brototyp I'll see what I can do. I think running carthage build --no-skip-current would be a first step, since it's what they recommend as a first diagnostic.

elitalon commented 7 years ago

@brototyp There's a problem with TrackerSpec.swift:70when running tests on iOS 9.3:

let _ = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { timer in
    trackerFixture.tracker.queue(event: EventFixture.event())
}

Basically, scheduledTimer(withTimeInterval:repeats:block:) is only available in iOS 10+. Do you have any suggestions on how to fix this?

A quick dirty solution would be using GCD:

if #available(iOS 10, *) {
    let _ = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true) { timer in
        trackerFixture.tracker.queue(event: EventFixture.event())
    }
} else {
    var scheduleEventTracking: (() -> Void)?
    scheduleEventTracking = {
        guard numberOfDispatches < 5 else {
            return
        }

        DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) {
            trackerFixture.tracker.queue(event: EventFixture.event())
            scheduleEventTracking?()
        }
    }
    scheduleEventTracking?()
}

What do you think?

brototyp commented 7 years ago

@elitalon Your solution might result in wrong test fails. If the trackerFixture.tracker.queue happens two times in between dispatches there will only be 4 dispatches in total and the test fails. I will fix this issue in a separate test branch.

brototyp commented 7 years ago

@elitalon What do you think about #179?

elitalon commented 7 years ago

@brototyp I already commented on #179, much better and without branching on iOS versions. As soon as it gets merged I'll rebase here.

brototyp commented 7 years ago

Great, thanks! #179 is merged.

elitalon commented 7 years ago

@brototyp The PR is again ready, but something happened with the Travis build. Do you have any idea? I don't have rights to trigger it again.

brototyp commented 7 years ago

@elitalon I just started the build once again (a third time) and it all went through. Thanks!