Feature/custom variables #205
Conversation
|
|
||
| extension PiwikTracker { | ||
|
|
||
| internal func getDefaultCVars() -> [CustomVariable] { |
There was a problem hiding this comment.
What do you think about moving this to the CustomVariable struct? Something like this:
extension CustomVariable {
static var defaultVariables: [CustomVariable] {
// return ...
}
}| @@ -36,6 +36,8 @@ final public class PiwikTracker: NSObject { | |||
|
|
|||
| internal var dimensions: [CustomDimension] = [] | |||
|
|
|||
| @objc public var useDefaultCustomVariables: Bool = false | |||
| private lazy var cvars: [CustomVariable] = getDefaultCVars() | |||
There was a problem hiding this comment.
I don't like the cvars variable name here to much. We usually don't use abbreviations. What do you think about using customVariables here? This then shows another minor issue. It is imho hard to distinguish (just by the variable names) between the cvars and the customVariables that you implemented as a slice from these here.
What do you think about removing the useDefaultCustomVariables completely and let the users of the SDK add the "default" variables themselves? Exposing a defaultVariables property on the CustomVariable for example could help here. This way, the code here in the SDK is much cleaner and it is more direct for the user.
| /// Note that, as with any array, the index of any succeeding Custom Variables is decremented by 1. | ||
| /// | ||
| /// - Parameter index: The index that was previously returned by addCustomVariable(). | ||
| @objc public func removeCustomVariableAtIndex(_ index: Int) { |
There was a problem hiding this comment.
What do you think about referencing the variables by name, not by index? In this case I think the CustomVariable struct should bring the index to be referenced in the API.
@objc public func removeCustomVariable(withName name: String) {
cvars = cvars.filter{$0.name != name }
}| var queryItems: [URLQueryItem] { | ||
| get { | ||
| let items = [ | ||
| var items = [ |
There was a problem hiding this comment.
Can you keep this as a let and the join al the different query items together in the end?
|
|
||
| private func cvarParameterValue() -> String { | ||
| var cvars: Array<String> = customVariables.enumerated().map { "\"\($0.offset + 1)\":[\"\($0.element.name)\",\"\($0.element.value)\"]" } | ||
| return "{\(cvars.joined(separator: ","))}" |
There was a problem hiding this comment.
I think this code is a bit to complex to parse. Do you think it is possible to optimize it?
|
I would appreciate if you add this feature. |
|
I will. There is only a few little things left. @zantoku let me know if you don't agree with some of my comments or if you can't find any time for them. |
This Pull Request adds support for Custom Variables, largely following the implementation pattern of Custom Dimensions. Custom Variables can be set scoped to visit (as a property of the shared PiwikTracker) or to an Event. Also, the tracker now generates the same default variables (Platform, OS Version, App Version) as the old 3.x tracker.
Also, I've improved the ObjC-interoperability with Custom Dimensions, by adding a setter method that can be called from there.