implemented URLSessionDispatcher #122
Conversation
|
Cleanup ist done. I also adapted some of the Example Application. The |
| // | ||
| // Created by Cornelius Horstmann on 25.02.17. | ||
| // Copyright © 2017 Mattias Levin. All rights reserved. | ||
| // |
There was a problem hiding this comment.
I thought we said to remove those comments?!
There was a problem hiding this comment.
That is true.
- Remove the comments in the header
| if (buttonIndex == 0) { | ||
| [PiwikTracker sharedInstance].optOut = YES; | ||
| } else { | ||
| [PiwikTracker sharedInstance].optOut = NO; |
There was a problem hiding this comment.
Do we have/need tickets/issues for those things that are currently not supported bei the swift version of piwik?
There was a problem hiding this comment.
I have just written a ticket #124
| spec.version = "3.3.0" | ||
| spec.summary = "A Piwik tracker written in Objective-C for iOS and OSX apps." | ||
| spec.version = "4.0.0-alpha1" | ||
| spec.summary = "A Piwik tracker written in Swift for iOS and OSX apps." |
There was a problem hiding this comment.
What about watchOS and tvOS? Should piwik support those, too?
There was a problem hiding this comment.
I would say we start with iOS and add macOS and tvOS later. I don't think it will be a huge issue though.
| } | ||
| set { | ||
| userDefaults.setValue(newValue, forKey: PiwikUserDefaults.Key.visitorID) | ||
| userDefaults.synchronize() |
There was a problem hiding this comment.
Do we really need this synchronize?
There was a problem hiding this comment.
Sure. Otherwise we might lose the values we just set. I think we should not risk using the values in case of an early exit (especially an app crash). What do you think @thorstenstark ?
PiwikTracker/Tracker.swift
Outdated
|
|
||
| // shared instance | ||
| extension Tracker { | ||
| public static var sharedInstance: Tracker? { |
There was a problem hiding this comment.
The Swift way for naming shared instances is usually shared, isn't it?
There was a problem hiding this comment.
Great comment?
- change the shared instance property to
sharedinstead ofsharedInstance
PiwikTracker/Tracker.swift
Outdated
| } | ||
|
|
||
| extension Tracker { | ||
| public func track(view: [String]) { |
There was a problem hiding this comment.
As this could be multiple views, I would call it track(views: [String]).
There was a problem hiding this comment.
I actually is not tracking multiple views. A view can be described by a path/categories. For example videos/cats/15.
Your question definitely shows an issue. All public methods, especially the Tracker, should be documented.
- Document public Tracker methods.
| let task = session.dataTask(with: request) { data, response, error in | ||
| // should we check the response? | ||
| // let dataString = String(data: data!, encoding: String.Encoding.utf8) | ||
| if error == nil { |
There was a problem hiding this comment.
Does the response perhaps tell us something we should know/handle?
There was a problem hiding this comment.
As far as I know, the result is only interesting when debugging the sdk. The information it returns possibly helps finding sdk-issues.
What do you think about having a debug property in the Tracker itself which is false per default? If it is set, we parse the data and will print it to the console.
PiwikTracker/Tracker.swift
Outdated
| public class func configureSharedInstance(withSiteID siteID: String, baseURL: URL) { | ||
| let queue = MemoryQueue() | ||
| let dispatcher = URLSessionDispatcher(baseURL: baseURL) | ||
| self._sharedInstance = Tracker.init(siteId: siteID, queue: queue, dispatcher: dispatcher) |
There was a problem hiding this comment.
Somehow this feels kind of strange setting the _sharedInstance in a class function... But I don't have a better solution for now.
Alright. This is the first version of the URLSessionDispatcher. I think it is ready now and maybe needs some cleanup.