Skip to content

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented Aug 29, 2023

Fixes: #issue

Motivation and Context

Adds support for typed response functions.

Description

  • enable strict null checks
  • Keeps getAssignment(...): string
  • Adds getStringAssignment, getBoolAssignment, getNumericAssignment, getJSONAssignment
  • Created EppoValue class

How has this been tested?

unit tests

@leoromanovsky leoromanovsky marked this pull request as draft August 31, 2023 05:05
@leoromanovsky leoromanovsky force-pushed the lr/dynamic-config branch 3 times, most recently from 5b5950a to c42ac0d Compare August 31, 2023 07:25
@leoromanovsky leoromanovsky marked this pull request as ready for review August 31, 2023 07:28
Comment on lines +101 to +105
assignmentHooks?.onPostAssignment(experimentKey, subjectKey, assignment);

if (assignment !== null)
this.logAssignment(experimentKey, assignment, subjectKey, subjectAttributes);

Copy link
Member Author

Choose a reason for hiding this comment

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

the pre and post assignment events caused a bunch of duplicate code in these handlers

"target": "es2017",
"outDir": "dist",
"noImplicitAny": true,
"strictNullChecks": true,

Choose a reason for hiding this comment

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

👍

@agamkrbit
Copy link

@leoromanovsky Can we test it with the updated test data from the GitHub Repository?

@leoromanovsky
Copy link
Member Author

leoromanovsky commented Sep 1, 2023

Thanks Agam for the suggestion to add the new test cases the latest ones brought up a bug that I corrected.

I also had a hard time getting the new test-data command working as it is written in the java repo; seemed like a race condition was happening where git clone wasn't getting waited on and the script crashing trying to copy a json file. I've fixed it with the version as written:

testDataDir := test/data/
tempDir := ${testDataDir}temp/
gitDataDir := ${tempDir}sdk-test-data/
branchName := main
githubRepoLink := https://github.com/Eppo-exp/sdk-test-data.git
repoName := sdk-test-data
.PHONY: test-data
test-data: 
	rm -rf $(testDataDir)
	mkdir -p $(tempDir)
	git clone -b ${branchName} --depth 1 --single-branch ${githubRepoLink} ${gitDataDir}
	cp ${gitDataDir}rac-experiments-v3.json ${testDataDir}
	cp -r ${gitDataDir}assignment-v2 ${testDataDir}
	rm -rf ${tempDir}

👉 Last bit of "clean up" I'd like to do here is try to unify the IValue and EppoValue types - will do a bit of research to see what's possible with json deserialization.

https://medium.com/@aems/one-mans-struggle-with-typescript-class-serialization-478d4bbb5826

gsutil cp gs://sdk-test-data/rac-experiments-v3.json $(testDataDir)
gsutil cp -r gs://sdk-test-data/assignment-v2 $(testDataDir)
mkdir -p $(tempDir)
git clone -b ${branchName} --depth 1 --single-branch ${githubRepoLink} ${gitDataDir}
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 +316 to +317
if (ba === null) return null;
return EppoValue.Bool(ba);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor but with short-circuiting and I think we could simplify this to return ba && EppoValue.Bool(ba).

> v = null
null
> v && Bool(v)
null
> v = true
true
> v && Boolean(v)
true

@@ -0,0 +1,28 @@
import { EppoValue } from './eppo_value';

describe('EppoValue toString function', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

@aarsilv aarsilv removed their assignment Sep 5, 2023
@leoromanovsky leoromanovsky merged commit 8d375a4 into main Sep 5, 2023
@typotter typotter deleted the lr/dynamic-config branch March 12, 2025 16:19
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.

3 participants