feat: get variation details by variation type #90

duyhungtnn · 2024-06-22T09:13:58Z

Changes

Added a new data class BKTEvaluationDetail<T> to support getting variation details by variation type.
Added a new data class BKTValue to reference a JSON node value
Introduce a new interface to get variation details by variation type

func objectVariation(featureId: String, defaultValue: BKTValue) -> BKTValue
func intVariationDetails(featureId: String, defaultValue: Int) -> BKTEvaluationDetail<Int> {}
func doubleVariationDetails(featureId: String, defaultValue: Double) -> BKTEvaluationDetail<Double> {}
func boolVariationDetails(featureId: String, defaultValue: Bool) -> BKTEvaluationDetail<Bool> {}
func stringVariationDetails(featureId: String, defaultValue: String) -> BKTEvaluationDetail<String> {}
func objectVariationDetails(featureId: String, defaultValue: BKTValue) -> BKTEvaluationDetail<BKTValue> {}

The class BKTEvaluation & the methods getEvaluationDetails, jsonVariation will be deprecated after this PR.

duyhungtnn · 2024-06-23T09:48:56Z

@cre8ivejp please help me to take a look on this PR

kakcy

thank you!
I have made some comments, please check them out.

Bucketeer/Sources/Public/BKTClient.swift

kakcy · 2024-06-25T09:22:56Z

Bucketeer/Sources/Public/BKTEvaluation.swift

+    public let variationValue: T
+    public let reason: Reason
+
+    public enum Reason: String, Codable, Hashable {


Although it is similar to the ReasonType definition, it is not very desirable to have the same definition in multiple places, so I would like you to consider using it uniformly if possible. Or consider removing the definition if you don't need it.

ReasonType

is for internal use only. I maintain the same approach like before

I would like to keep this. The Android SDK has something like this as well. @kakcy

kakcy · 2024-06-25T09:25:54Z

Bucketeer/Sources/Public/BKTEvaluation.swift

+        case offVariation = "OFF_VARIATION"
+        case prerequisite = "PREREQUISITE"
+
+        public static func fromString(value: String) -> Reason {


It might be good to have a test for the fromString().

I will add them

updated in d170ebe

Bucketeer/Sources/Public/BKTEvaluation.swift

kakcy · 2024-06-25T09:51:17Z

Bucketeer/Sources/Public/BKTClient.swift

@@ -200,6 +238,7 @@ extension BKTClient {
        }
    }

+    @available(*, deprecated, message: "use stringEvaluationDetails<T> instead")


??

Suggested change

@available(*, deprecated, message: "use stringEvaluationDetails<T> instead")

@available(*, deprecated, message: "use getVariationDetail<T> instead")

hi @kakcy ,

getVariationDetail is for internal use only. It can not be accessed from outside.
But I will remote the <T> from the stringEvaluationDetails<T>

I updated it to

@available(*, deprecated, message: "use stringVariationDetails(featureId:, defaultValue:) instead")

Co-authored-by: kakcy <43775118+kakcy@users.noreply.github.com>

duyhungtnn · 2024-06-25T14:34:00Z

@kakcy thank you for reviewing.
I updated the PR with some changes from your suggestion.
Also, I changed the interface a little in this commit 12cbd20

as they are a new requirement bucketeer-io/android-client-sdk#180 (comment)

kakcy

Thank you for your responses 👍

cre8ivejp

Thank you!
I'll merge next week.

duyhungtnn · 2024-08-02T01:48:35Z

hi @cre8ivejp , @kakcy
Please help me to take a look at this PR.
I added changes from the latest review. These changes make our interface closest to OpenFeatue specification

Changes

Added a new data class BKTValue to reference a JSON node value
Added a new method to return a BKTValue

public func objectVariationDetails(featureId: String, defaultValue:BKTValue)

Removed jsonVariationDetail
Deprecated the method jsonVariation

duyhungtnn · 2024-08-03T08:51:55Z

Although this PR seems ready for review. However, I found some edge cases when finalizing the same changes in the Android PR.

I will add some new tests to this PR.

…s a double

duyhungtnn · 2024-08-21T07:02:46Z

New changes

Added support to return intValue even when the underlying value is a double 8d576f2
Created BKTValue.number to hold JSON number value 58a6bb7

duyhungtnn · 2024-08-26T10:14:30Z

New changes

keep the detail interface right after the non-detail interface 3515534

Bucketeer/Sources/Public/BKTClient.swift

cre8ivejp · 2024-09-05T00:04:45Z

Bucketeer/Sources/Public/BKTEvaluation.swift

+public struct BKTEvaluationDetails<T:Equatable>: Equatable {
+    public let featureId: String
+    public let featureVersion: Int
+    public let userId: String
+    public let variationId: String
+    public let variationName: String
+    public let variationValue: T
+    public let reason: Reason
+
+    public enum Reason: String, Codable, Hashable {


@duyhungtnn, WDYT of adding the BKTEvaluationDetails to a new file like you did in the Android SDK?

It's good.
I updated 6e761ac

cre8ivejp

Thank you!

feat: new detailed evaluation interface

15334cf

duyhungtnn self-assigned this Jun 22, 2024

duyhungtnn added 6 commits June 22, 2024 16:28

feat: add type generic method to new interface

802bee6

fix: swiftlint

2afc9f5

test: init new test BKTClientEvaluationDetailTests

05a9729

test: full unit tests for new interface

7f56f7b

fix: test fail

55694c8

fix: test json evaluation detail fail

7f724cb

duyhungtnn marked this pull request as ready for review June 23, 2024 09:19

duyhungtnn requested a review from cre8ivejp June 23, 2024 09:19

duyhungtnn added 2 commits June 23, 2024 16:24

fix: remove incorrect type check

a17939c

fix: update deprecated message

15d209e

chore: correct grammar

5d46c4b

cre8ivejp requested a review from kakcy June 25, 2024 07:06

kakcy reviewed Jun 25, 2024

View reviewed changes

duyhungtnn and others added 7 commits June 25, 2024 17:50

Update Bucketeer/Sources/Public/BKTEvaluation.swift

df5b19b

Co-authored-by: kakcy <43775118+kakcy@users.noreply.github.com>

Update Bucketeer/Sources/Public/BKTEvaluation.swift

418ec10

Co-authored-by: kakcy <43775118+kakcy@users.noreply.github.com>

Update Bucketeer/Sources/Public/BKTClient.swift

b5caee8

Co-authored-by: kakcy <43775118+kakcy@users.noreply.github.com>

chore: correct deprecated message

fa20eb0

test: for Reason.fromString

d170ebe

chore: update new interface method names

12cbd20

chore: correct message text

d990311

duyhungtnn requested a review from kakcy June 25, 2024 14:34

kakcy previously approved these changes Jun 26, 2024

View reviewed changes

cre8ivejp changed the title ~~feat: new detailed evaluation interface~~ feat: get evaluation details interface Jun 26, 2024

cre8ivejp changed the title ~~feat: get evaluation details interface~~ feat: get variation details by variation type Jun 26, 2024

cre8ivejp previously approved these changes Jul 5, 2024

View reviewed changes

cre8ivejp marked this pull request as draft July 5, 2024 12:25

duyhungtnn dismissed stale reviews from cre8ivejp and kakcy via 8ac1c4f July 31, 2024 02:38

duyhungtnn force-pushed the feat/new-detailed-evaluation-interface branch from 9e2ecd9 to d990311 Compare August 1, 2024 08:47

duyhungtnn added 3 commits August 1, 2024 22:29

feat: support JSON Array

7859d34

chore: update comments

f54b4a5

test: add BKTValue tests

e0cb9c8

duyhungtnn marked this pull request as ready for review August 2, 2024 01:16

duyhungtnn requested review from cre8ivejp and kakcy August 2, 2024 01:43

duyhungtnn added 2 commits August 2, 2024 20:50

chore: make logic to decode BKTValue more simpler

39c10a4

chore: add test testGetVariationValueAsBKTValue

8bcadc4

duyhungtnn added 2 commits August 8, 2024 10:47

tests: update BKTValueTests

3d3bad1

feat: Add support to return intValue even when the underlying value i…

8d576f2

…s a double

duyhungtnn added 3 commits August 21, 2024 15:31

feat: create BKTValue.number to hold JSON number value

58a6bb7

tests: add more test for BKTValueTests.swift

f4b15bb

chore: keep the detail interface right after the non-detail interface

3515534

cre8ivejp reviewed Aug 27, 2024

View reviewed changes

Bucketeer/Sources/Public/BKTClient.swift Outdated Show resolved Hide resolved

duyhungtnn added 3 commits August 28, 2024 16:47

chore: rename getVariationDetail to getBKTEvaluationDetails

3ad3ed3

Merge branch 'main' into feat/new-detailed-evaluation-interface

9dcbf00

fix: build fail

7a0907e

cre8ivejp reviewed Sep 5, 2024

View reviewed changes

chore: move BKTEvaluationDetails to a new file

6e761ac

duyhungtnn requested a review from cre8ivejp September 5, 2024 04:15

cre8ivejp approved these changes Sep 5, 2024

View reviewed changes

cre8ivejp merged commit 5a79914 into main Sep 5, 2024
7 checks passed

cre8ivejp deleted the feat/new-detailed-evaluation-interface branch September 5, 2024 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: get variation details by variation type #90

feat: get variation details by variation type #90

duyhungtnn commented Jun 22, 2024 •

edited

Loading

duyhungtnn commented Jun 23, 2024

kakcy left a comment

kakcy Jun 25, 2024

duyhungtnn Jun 25, 2024

duyhungtnn Jun 25, 2024

kakcy Jun 25, 2024

duyhungtnn Jun 25, 2024

duyhungtnn Jun 25, 2024

kakcy Jun 25, 2024

duyhungtnn Jun 25, 2024

duyhungtnn Jun 25, 2024 •

edited

Loading

duyhungtnn commented Jun 25, 2024

kakcy left a comment

cre8ivejp left a comment

duyhungtnn commented Aug 2, 2024 •

edited

Loading

duyhungtnn commented Aug 3, 2024

duyhungtnn commented Aug 21, 2024 •

edited

Loading

duyhungtnn commented Aug 26, 2024

cre8ivejp Sep 5, 2024

duyhungtnn Sep 5, 2024

cre8ivejp left a comment

	@available(*, deprecated, message: "use stringEvaluationDetails<T> instead")
	@available(*, deprecated, message: "use getVariationDetail<T> instead")

feat: get variation details by variation type #90

feat: get variation details by variation type #90

Conversation

duyhungtnn commented Jun 22, 2024 • edited Loading

Changes

duyhungtnn commented Jun 23, 2024

kakcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duyhungtnn Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

duyhungtnn commented Jun 25, 2024

kakcy left a comment

Choose a reason for hiding this comment

cre8ivejp left a comment

Choose a reason for hiding this comment

duyhungtnn commented Aug 2, 2024 • edited Loading

Changes

duyhungtnn commented Aug 3, 2024

duyhungtnn commented Aug 21, 2024 • edited Loading

New changes

duyhungtnn commented Aug 26, 2024

New changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cre8ivejp left a comment

Choose a reason for hiding this comment

duyhungtnn commented Jun 22, 2024 •

edited

Loading

duyhungtnn Jun 25, 2024 •

edited

Loading

duyhungtnn commented Aug 2, 2024 •

edited

Loading

duyhungtnn commented Aug 21, 2024 •

edited

Loading