-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/metadata #5
Feature/metadata #5
Conversation
This corrects an issue where all visits are coming through as unique. This appears to be because the updated date is not being written back to the userDefaults
Thanks, @chazchazchaz, for adding this to the package. Very helpful! I'm no Swift expert, so I'll wait a few days on @roelvanderkraan before merging. I did merge the other oneline PR #4 |
I will take a good look this weekend, but at a first glance it looks good to me! Thanks @chazchazchaz! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chazchazchaz thanks again for this pull request. I've got some minor remarks. Most importantly about support of the 4 data types that Simpleanalytics meta data supports. The rest looks good to me!
|
||
let tracker = SimpleAnalytics(hostname: "simpleanalyticsswift.app") | ||
|
||
let metadataDictionary = ["plan": "premium", "meta": "data"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the SimpleAnalytics docs, meta data can have 4 types: text, date, boolean or number. I think it would be good to include these in the test dictionary (and in examples).
@@ -123,7 +142,7 @@ final public class SimpleAnalytics: NSObject { | |||
} else { | |||
// Last visit is not in today, so unique. | |||
self.visitDate = Date() | |||
UserDefaults.standard.set(visitDate, forKey: Keys.visitDateKey) | |||
UserDefaults.standard.set(self.visitDate, forKey: Keys.visitDateKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing!
do { | ||
let metadataData = try JSONSerialization.data(withJSONObject: metadataDictionary, options: []) | ||
let metadataJsonString = String(data: metadataData, encoding: .utf8)! | ||
tracker.trackEvent(event: "test", metadata: metadataJsonString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space character in front after metadata:
/// Serializes metadata dictionary into a JSON string. | ||
/// - Parameter metadata: The metadata dictionary, which is optional. | ||
/// - Returns: A JSON string representation of the metadata or nil if serialization fails or metadata is nil/empty. | ||
internal func metadataToJsonString(metadata: [String: Any]?) -> String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add an extra test case for your metadataToJsonString function.
Better late then never :) |
Support metadata