Skip to content
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

Another PR for Feature/custom variables #223

Merged
merged 11 commits into from
Jan 29, 2018

Conversation

manuroe
Copy link
Contributor

@manuroe manuroe commented Jan 29, 2018

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:

  • merge the @zantoku's PR into the current develop branch
  • take into account @brototyp's remarks except Feature/custom variables #205 (comment) because I have no idea how to make the string formatting in this piece of code more readable.
  • remove default custom variables created by the initial getDefaultCVars method because indexes and names used for these custom variables look like very specific to an app. I have not seen matomo using such default values in other SDKs. So, I have preferred to remove them and let the app set them.

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
Copy link
Contributor Author

manuroe commented Jan 29, 2018

The last point is implemented in #224.

@brototyp
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

manuroe commented Jan 29, 2018

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

@brototyp
Copy link
Member

Awesome! Thanks.

@brototyp brototyp merged commit db1a1dc into matomo-org:develop Jan 29, 2018
@brototyp
Copy link
Member

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

@manuroe
Copy link
Contributor Author

manuroe commented Jan 30, 2018

Do you want to be named there?

@brototyp, yes please.

This was referenced Jan 30, 2018
brototyp pushed a commit that referenced this pull request Feb 26, 2018
* PiwikTracker.isOptedOut property visible to objc

* custom variables added to Tracker and Event for per-visit and per-event scoping

* CustomVariables code documentation

* Objective-C accessibility for Custom Dimensions, Code documentation.

* Objective-C accessibility for PiwikTracker.dispatchInterval

* Custom Variable: add the "index" member and update PiwikTracker API to manage Custom Variables

* Custom Variable: Remove PiwikTracker.getDefaultCVars() as it is very app specific

* Custom Variable: rename cvars to customVariables

* Custom Variable: take into account remark at https://github.com/matomo-org/matomo-sdk-ios/pull/205/files#r154520132

* Custom Variable: Make EventSerializer.customVariableParameterValue() more readable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants