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

Another PR for Feature/custom variables #223

Closed manuroe closed 6 years ago

manuroe commented 6 years ago

Hi,

This PR is the continuation of the work made by @zantoku in PR #205, which is stuck because review remarks have not yet been taken into account.

What I have done in this PR:

This last point will require to make the Device struct accessible from objc in order to offer the same functionality as the initial PR. If you are ok, I will make it in another PR.

manuroe commented 6 years ago

The last point is implemented in https://github.com/matomo-org/matomo-sdk-ios/pull/224.

brototyp commented 6 years ago

Hi @manuroe, Awesome. Thank you for your work. To the getDefaultCVars: I think the code came from porting over the original, Objective-c code. Back then such a logic was implemented. I am very fine with keeping it as you implemented it and to deprecate such usage.

I will sketch around a bit regarding my comment about the readability.

brototyp commented 6 years ago

I thought about serializing it as JSON, but that adds another complexity. The best I could find is the following. What do you think?

private func customVariableParameterValue() -> String {
    let customVariableParameterValue: [String] = customVariables.map { "\"\($0.index)\":[\"\($0.name)\",\"\($0.value)\"]" }
    return "{\(customVariableParameterValue.joined(separator: ","))}"
}
manuroe commented 6 years ago

Your customVariableParameterValue is fine by me. I have updated the PR.

brototyp commented 6 years ago

Awesome! Thanks.

brototyp commented 6 years ago

@manuroe I've just seen, that there is no Changelog entry. Do you want to be named there?

manuroe commented 6 years ago

Do you want to be named there?

@brototyp, yes please.