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

feat: update iOS SDK #5

Merged
merged 31 commits into from
Jun 19, 2023
Merged

Conversation

duyhungtnn
Copy link
Collaborator

@duyhungtnn duyhungtnn commented Jun 14, 2023

Changes

  • Move ReasonType to internal scope
  • Send tags when tracking events
  • Fix duplicate event metrics logic
  • Expose BKTUser's properties
  • Make BKTError implement LocalizeError
  • Add more test cases to check duplicate metrics event

Refs

bucketeer-io/android-client-sdk#64
bucketeer-io/android-client-sdk#68

@duyhungtnn duyhungtnn marked this pull request as draft June 14, 2023 23:12
@duyhungtnn duyhungtnn marked this pull request as ready for review June 16, 2023 04:03
@duyhungtnn
Copy link
Collaborator Author

@cre8ivejp Hi, this PR is ready for review. Please help me to take a look. Let me know if you see any problems. Thank you.

@cre8ivejp cre8ivejp self-requested a review June 16, 2023 09:14
@cre8ivejp
Copy link
Member

@duyhungtnn

I think we don't need the // swiftlint:enable type_body_length here
Also, please use // swiftlint:disable file_length at the top of the file to fix the warning.

Please fix the warning in this line.
It should be as follows.

    init(
        apiEndpoint: URL,
        apiKey: String,
        ...

@@ -571,4 +571,4 @@ final class EventInteractorTests: XCTestCase {
wait(for: [expectation], timeout: 1)
}
}
// swiftlint:enable [type_body_length file_length]
// swiftlint:enable type_body_length file_length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming. Does it need to enable at the end of the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need to re-enable it at the end of the file, otherwise we will have one more warning like that

Screenshot 2023-06-19 at 10 46 24

@@ -94,4 +94,68 @@ extension BKTError {
self = .unknown(message: "Unknown error: \(error)", error: error)
}
}

/// A localized message describing what error occurred.
public var errorDescription: String? {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw a problem when updating Flutter SDK, I need many switch cases to read the error.
So that I added the code below :
-> Support SDK's consumer read error messages the easy way.
@cre8ivejp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change need to be done also in Android SDK?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will check when I am updating the Flutter for Android and send new PR to Android SDK if needed

let id: String
let attr: [String: String]
public let id: String
public let attr: [String: String]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BKUser's properties are internal access so that the SDK's consumer (Flutter) cannot access the current user data.
It makes BKTClient.getCurrentUser() useless

@cre8ivejp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing it!

@@ -571,4 +571,4 @@ final class EventInteractorTests: XCTestCase {
wait(for: [expectation], timeout: 1)
}
}
// swiftlint:enable [type_body_length file_length]
// swiftlint:enable type_body_length file_length
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we need to re-enable it at the end of the file, otherwise we will have one more warning like that

Screenshot 2023-06-19 at 10 46 24

// MetricsEventDataTests.swift
// BucketeerTests
//
// Created by Ryan Hung Pham on 15/06/2023.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete the header. I'm planning to run a script to add it to all files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the header

Copy link
Member

@cre8ivejp cre8ivejp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@cre8ivejp cre8ivejp merged commit c62bc8e into bucketeer-io:main Jun 19, 2023
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