fix: added reusable code for wrapper SDKs #247

Shahroz16 · 2023-01-10T02:54:59Z

Complete each step to get your pull request merged in. Learn more about the workflow this project uses.

Assign members of your team to review the pull request.
Wait for pull request status checks to complete. If there are problems, fix them until you see that all status checks are passing.
Wait until the pull request has been reviewed and approved by a teammate
After pull request is approved, and you determine it's ready add the label "Ready to merge" to the pull request. A bot will squash and merge the pull request for you after the label is added.

- extensions - initializer with config params

ami-oss-ci · 2023-01-10T02:55:13Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.0.1. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

To merge this pull request, add the label Ready to merge to this pull request and I'll merge it for you.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

codecov · 2023-01-10T03:02:04Z

Codecov Report

Merging #247 (45f4bbd) into develop (81a8318) will decrease coverage by 0.58%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #247      +/-   ##
===========================================
- Coverage    53.48%   52.90%   -0.58%     
===========================================
  Files          101      102       +1     
  Lines         1120     1533     +413     
===========================================
+ Hits           599      811     +212     
- Misses         521      722     +201

Impacted Files	Coverage Δ
Sources/Tracking/CustomerIO.swift	`74.19% <ø> (ø)`
Sources/Common/Service/Request/PushMetric.swift	`100.00% <100.00%> (ø)`
Sources/Common/Store/SdkConfig.swift	`60.00% <100.00%> (-6.67%)`	⬇️
Sources/Common/Type/Region.swift	`100.00% <100.00%> (ø)`
Sources/Common/Util/Log.swift	`90.00% <100.00%> (+1.11%)`	⬆️
...ngInApp/autogenerated/AutoMockable.generated.swift	`67.74% <0.00%> (-4.49%)`	⬇️
...PushFCM/autogenerated/AutoMockable.generated.swift	`28.20% <0.00%> (-3.62%)`	⬇️
...PushAPN/autogenerated/AutoMockable.generated.swift	`28.94% <0.00%> (-2.88%)`	⬇️
...ingPush/autogenerated/AutoMockable.generated.swift	`0.00% <0.00%> (ø)`
...ogenerated/AutoDependencyInjection.generated.swift	`100.00% <0.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Sources/Common/Extensions/StringExtensions.swift

Sources/Common/Store/SdkConfig.swift

Sources/Tracking/CustomerIO.swift

Sources/Tracking/CustomerIOInstance.swift

Sources/Common/Util/CustomerIOBuilderConfigKeys.swift

Sources/Common/Store/SdkConfig.swift

Sources/Common/Util/Log.swift

Sources/Tracking/CustomerIO.swift

levibostian · 2023-01-18T20:37:53Z

Could you add automated tests to this PR to cover the code that is added?

Shahroz16 · 2023-01-19T06:23:15Z

Could you add automated tests to this PR to cover the code that is added?

@levibostian I have added already the tests, can you please explain what automated tests are you referring to?

levibostian

My apologies, I didn't see tests written at first. I take back my comment asking for automated tests as I see them now.

One of my biggest concerns about the changes made to SdkConfig with being able to create SdkConfig object from [String: Any] is that someone may forget to update SdkConfig.Factory.create to allow params to set that modified SdkConfig property.

Example: We add a new SdkConfig property enableInApp: Bool. We could quite easily forget to update SdkConfig.Factory.create to not include params["enableInApp"] to enable in-app with SDK wrappers.

I wasn't sure on a good solution to prevent this potential human error so I decided to play with the code to come up with a solution.

I made a commit to this branch with a suggestion. I added comments to that commit to highlight the logic behind it. To summarize, I modified the SdkConfig construction so if you ever add/remove/edit a property inside of SdkConfig object, the Swift compiler throws an error in SdkConfig.Factory.create reminding you that you need to add a params override and default value there.

Sources/Common/Store/SdkConfig.swift

Sources/Tracking/CustomerIO.swift

Tests/Common/Extensions/CIOExtensionsTest.swift

levibostian · 2023-01-19T17:12:10Z

Tests/Common/Extensions/CIOExtensionsTest.swift

+    func test_region_givenString_expectRegion() {
+        let givenUSRegion = Region.US
+        let expectedUSRegion = Region.getRegion(from: "us")
+
+        XCTAssertEqual(givenUSRegion, expectedUSRegion)
+
+        let givenEURegion = Region.EU
+        let expectedEURegion = Region.getRegion(from: "EU")
+
+        XCTAssertEqual(givenEURegion, expectedEURegion)
+    }


I like how this function is testing both uppercase and lowercase. I think that's good to test. To help us make sure that we continue updating this edge case, I suggest we break apart into 2 test functions that are named to suggest the edge case we are trying to test.

Suggested change

func test_region_givenString_expectRegion() {

let givenUSRegion = Region.US

let expectedUSRegion = Region.getRegion(from: "us")

XCTAssertEqual(givenUSRegion, expectedUSRegion)

let givenEURegion = Region.EU

let expectedEURegion = Region.getRegion(from: "EU")

XCTAssertEqual(givenEURegion, expectedEURegion)

}

func test_from_givenUppercaseString_expectGetRegion() {

let given = Region.getRegion(from: "US")

let expected = Region.US

XCTAssertEqual(given, expected

}

func test_from_givenLowercaseString_expectGetRegion() {

let given = Region.getRegion(from: "us")

let expected = Region.US

XCTAssertEqual(given, expected

}

Since Region.getRegion(from)'s implementation is using the rawValue of Region, I think that this would be a good test function to add:

Suggested change

func test_region_givenString_expectRegion() {

let givenUSRegion = Region.US

let expectedUSRegion = Region.getRegion(from: "us")

XCTAssertEqual(givenUSRegion, expectedUSRegion)

let givenEURegion = Region.EU

let expectedEURegion = Region.getRegion(from: "EU")

XCTAssertEqual(givenEURegion, expectedEURegion)

}

func test_from_expectConvertToAndFromRegion() {

let expected = Region.US

let actual = Region.getRegion(from: Region.US.rawValue)

XCTAssertEqual(expected, actual)

}

Tests/Common/Extensions/CIOExtensionsTest.swift

Sources/Tracking/CustomerIO.swift

ami-oss-ci · 2023-01-19T20:58:02Z

	Warnings
⚠️	I noticed file `Tests/Tracking/APITest.swift` was modified. That could mean that this pull request is introducing a breaking change to the SDK. If this pull request does introduce a breaking change, make sure the pull request title is in the format: `<type>!: description of breaking change // Example: refactor!: remove onComplete callback from async functions`

Generated by 🚫 dangerJS against 45f4bbd

levibostian · 2023-01-19T21:00:54Z

I am sorry Shahroz if me pushing a commit to the branch caused confusion or frustration compared to an alternative action such as typing a suggestion in a comment in the PR.

Sometimes I am most productive by digging into the code instead of giving up a concept. Commits can always be modified and reverted anyway 😉.

I see that I broke tests by my push. I didn't consider that happening and I made a commit to attempt to fix them.

Trying to make this PR a collaborative experience. We're learning on how best to do that.

levibostian · 2023-01-19T21:05:03Z

Tests should be passing now.

From the tests I wrote I found a couple bugs 👍!

Shahroz16 · 2023-01-20T05:01:44Z

@levibostian thanks for the suggestions and the commit, the only confusion it causes was the implementation was a bit different than one of the suggested comments, for example, the Keys enum structure, the commit removed the enum completely.

The only pending comment seems to be regarding the initialization, so I'll update that, but it would also affect the changes you did it commit. So once you have time to review new changes, we can revisit it.

Tests/Tracking/APITest.swift

Tests/Common/Util/LogTest.swift

Sources/Common/Util/Log.swift

Sources/Tracking/CustomerIO.swift

Sources/Common/Store/SdkConfig.swift

Tests/Tracking/APITest.swift

## [2.1.0-beta.1](2.0.4...2.1.0-beta.1) (2023-02-07) ### Features * add in-app event listener ([#211](#211)) ([737d43b](737d43b)) * in-app feature no longer requires orgId ([#252](#252)) ([acd12da](acd12da)) ### Bug Fixes * added reusable code for wrapper SDKs ([#247](#247)) ([36adf15](36adf15)) * in-app missing event ([#259](#259)) ([43b3e97](43b3e97)) * modify in-app event listener action parameters to new name ([#255](#255)) ([b46528a](b46528a)) * region visibility modifier to be used by wrappers ([#260](#260)) ([f0edfbc](f0edfbc)) * update the gist version in podspec ([#256](#256)) ([5451488](5451488))

## [2.1.0](2.0.6...2.1.0) (2023-02-22) ### Features * add in-app event listener ([#211](#211)) ([737d43b](737d43b)) * in-app feature no longer requires orgId ([#252](#252)) ([acd12da](acd12da)) ### Bug Fixes * access modifier for metric ([#263](#263)) ([e641982](e641982)) * added reusable code for wrapper SDKs ([#247](#247)) ([36adf15](36adf15)) * in-app missing event ([#259](#259)) ([43b3e97](43b3e97)) * modify in-app event listener action parameters to new name ([#255](#255)) ([b46528a](b46528a)) * region visibility modifier to be used by wrappers ([#260](#260)) ([f0edfbc](f0edfbc)) * update the gist version in podspec ([#256](#256)) ([5451488](5451488))

github-actions · 2023-02-22T20:12:58Z

🎉 This PR is included in version 2.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Shahroz16 added 2 commits January 9, 2023 21:49

added reusable code for wrappers

f8b2bc4

- extensions - initializer with config params

code refactor

3d703a8

Shahroz16 added 2 commits January 9, 2023 22:35

linting issues resolved

bffca74

import reorder

de686e9

Shahroz16 requested a review from a team January 10, 2023 03:40

levibostian suggested changes Jan 12, 2023

View reviewed changes

Shahroz16 added 2 commits January 12, 2023 22:57

code refactor and PR suggestions

480c965

tests added

700c73d

Shahroz16 requested a review from levibostian January 13, 2023 12:28

Shahroz16 added 3 commits January 13, 2023 07:56

added config option for log as int

5ede497

lint

169779e

added more tests

0e310be

ami-aman reviewed Jan 16, 2023

View reviewed changes

Sources/Common/Store/SdkConfig.swift Outdated Show resolved Hide resolved

ami-aman reviewed Jan 16, 2023

View reviewed changes

Sources/Common/Util/Log.swift Outdated Show resolved Hide resolved

ami-aman reviewed Jan 16, 2023

View reviewed changes

Sources/Tracking/CustomerIO.swift Outdated Show resolved Hide resolved

Shahroz16 requested a review from ami-aman January 17, 2023 22:40

Shahroz16 added 2 commits January 19, 2023 01:18

added sdk config

aaf18b5

linting

c805708

refactor: compiler error if we modify SdkConfig properties in future

5f7e23c

levibostian reviewed Jan 19, 2023

View reviewed changes

tests: create test for sdkconfig with public map keys

ef1514e

fix tests

e53d08d

pr suggestions and code refactor

549b7f8

linting

85f8bb8

Shahroz16 requested a review from levibostian January 20, 2023 05:01

levibostian reviewed Jan 20, 2023

View reviewed changes

Shahroz16 added 5 commits January 20, 2023 22:45

pr suggestions

01c2f7f

linting

60a2676

changed modification pattern

f861859

linting

6404cee

lint fix

77a7c19

Shahroz16 requested a review from levibostian January 21, 2023 06:33

levibostian approved these changes Jan 23, 2023

View reviewed changes

Tests/Tracking/APITest.swift Outdated Show resolved Hide resolved

Update Tests/Tracking/APITest.swift

45f4bbd

Shahroz16 merged commit 36adf15 into develop Jan 24, 2023

Shahroz16 deleted the shahroz/initialization-with-config branch January 24, 2023 20:45

github-actions bot added the released label Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: added reusable code for wrapper SDKs #247

fix: added reusable code for wrapper SDKs #247

Shahroz16 commented Jan 10, 2023

ami-oss-ci commented Jan 10, 2023 •

edited

Loading

Examples:

codecov bot commented Jan 10, 2023 •

edited

Loading

levibostian commented Jan 18, 2023

Shahroz16 commented Jan 19, 2023

levibostian left a comment

levibostian Jan 19, 2023

levibostian Jan 19, 2023

ami-oss-ci commented Jan 19, 2023 •

edited

Loading

levibostian commented Jan 19, 2023

levibostian commented Jan 19, 2023

Shahroz16 commented Jan 20, 2023

github-actions bot commented Feb 22, 2023

-    func test_region_givenString_expectRegion() {
-        let givenUSRegion = Region.US
-        let expectedUSRegion = Region.getRegion(from: "us")
-        XCTAssertEqual(givenUSRegion, expectedUSRegion)
-        let givenEURegion = Region.EU
-        let expectedEURegion = Region.getRegion(from: "EU")
-        XCTAssertEqual(givenEURegion, expectedEURegion)
-    }
+    func test_from_givenUppercaseString_expectGetRegion() {
+        let given = Region.getRegion(from: "US")
+        let expected = Region.US
+        XCTAssertEqual(given, expected
+    }
+    func test_from_givenLowercaseString_expectGetRegion() {
+        let given = Region.getRegion(from: "us")
+        let expected = Region.US
+        XCTAssertEqual(given, expected
+    }

fix: added reusable code for wrapper SDKs #247

fix: added reusable code for wrapper SDKs #247

Conversation

Shahroz16 commented Jan 10, 2023

ami-oss-ci commented Jan 10, 2023 • edited Loading

Examples:

codecov bot commented Jan 10, 2023 • edited Loading

Codecov Report

levibostian commented Jan 18, 2023

Shahroz16 commented Jan 19, 2023

levibostian left a comment

Choose a reason for hiding this comment

levibostian Jan 19, 2023

Choose a reason for hiding this comment

levibostian Jan 19, 2023

Choose a reason for hiding this comment

ami-oss-ci commented Jan 19, 2023 • edited Loading

levibostian commented Jan 19, 2023

levibostian commented Jan 19, 2023

Shahroz16 commented Jan 20, 2023

github-actions bot commented Feb 22, 2023

ami-oss-ci commented Jan 10, 2023 •

edited

Loading

codecov bot commented Jan 10, 2023 •

edited

Loading

ami-oss-ci commented Jan 19, 2023 •

edited

Loading