Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

vpai
Copy link
Contributor

@vpai vpai commented Feb 14, 2024

Motivation and Context

Eppo's RAC API will start returning targeting rule values containing semver strings. Our SDKs need to be able to parse these strings and perform rule evaluation on them. Some examples of other SDKs:

Eppo-exp/js-sdk-common#38
Eppo-exp/eppo-ios-sdk#10

Description

The operations lt, gt, lte, gte apply to both numeric values and semver strings. Previously it was only numeric values.

Behavior of un-upgraded SDKs

They will continue to assume that only numeric values will be present for the lt, gt, lte, gte operations.

@vpai vpai force-pushed the varun/semver branch 4 times, most recently from e75e242 to 8990bae Compare February 14, 2024 23:00
@vpai vpai requested a review from leoromanovsky February 14, 2024 23:02
Copy link
Member

@leoromanovsky leoromanovsky left a comment

Choose a reason for hiding this comment

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

very nice! appreciate the unit test

Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

Great stuff! 👏

Just got one suggestion on making slightly trickier test data--although based on the code as it's written now it will most certainly pass.

return False


def is_valid_semver(value: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 Clean 🧼

return isinstance(
subject_value, numbers.Number
) and evaluate_numeric_condition(subject_value, condition)
# Numeric operator: value could be numeric or semver.
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the comments 🙌

assert find_matching_rule({"age": 99}, [rule]) == rule


def test_find_matching_rule_with_semver():
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

Comment on lines +73 to +75
assert find_matching_rule({"version": "1.1.0"}, [semver_rule]) is semver_rule
assert find_matching_rule({"version": "2.0.0"}, [semver_rule]) is semver_rule
assert find_matching_rule({"version": "2.1.0"}, [semver_rule]) is None
Copy link
Contributor

Choose a reason for hiding this comment

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

just for shits, to make sure more than just a string comparison is at play, consider having test data so that we do something like making sure "1.12.3" is greater than "1.2.0"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants