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: add reference to request structures #24

Merged
merged 14 commits into from
Jan 16, 2024
Merged

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Jan 10, 2024

Fixes FLI-726

We can hold off integrating this till the time is right.
But decided to get ahead of it.
This adds reference and optional parameter to all the request structures.

@GeorgeMac GeorgeMac requested a review from a team as a code owner January 10, 2024 17:13
@yquansah yquansah force-pushed the gm/thread-reference branch from eddb555 to 7511485 Compare January 10, 2024 20:42
Copy link

linear bot commented Jan 11, 2024

FLI-726 Server SDKs: Allow to pass a version/ref

Allow all server side SDKs to pin to a specific version or ref

@GeorgeMac
Copy link
Member Author

@markphelps should linear be commenting on this PR? Is that a product of the repo previously being private?

@markphelps
Copy link
Contributor

Yeah I think it's only because it was private previously. I don't think it will continue to do so going forward

@@ -12,11 +12,11 @@ public static void main(String[] args) {
context.put("fizz", "buzz");

EvaluationRequest variantEvaluationRequest =
new EvaluationRequest("default", "flag1", "entity", context);
new EvaluationRequest("default", "flag1", "entity", context, Optional.empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

im wondering if we should use the builder pattern here? since I would think most users wouldn't want to pass ref, and its a bit odd that we force them to pass Optional.empty() IMO

Copy link
Member Author

@GeorgeMac GeorgeMac Jan 16, 2024

Choose a reason for hiding this comment

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

Agreed, I think (based on Ferns approach) there is precedent for keeping this type how it is and adding a Builder as a sub-type for creating isntances of this final type. So this would still be a valid constructor, but there would be an additional builder to make this easier.
Does is make sense to tackle that as a general improvement in a subsequent PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah makes sense 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

#78

Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

PHP tho?

Copy link
Contributor

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

nice.php

@GeorgeMac GeorgeMac merged commit e202d50 into main Jan 16, 2024
6 checks passed
@GeorgeMac GeorgeMac deleted the gm/thread-reference branch January 16, 2024 16:24
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