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

Provider context type clarification #69

Closed
beeme1mr opened this issue Sep 16, 2022 · 5 comments · Fixed by #74
Closed

Provider context type clarification #69

beeme1mr opened this issue Sep 16, 2022 · 5 comments · Fixed by #74
Assignees

Comments

@beeme1mr
Copy link
Member

The FeatureProvider interface uses a map[string]interface{} instead of a EvaluationContext. Was this done intentionally?

This question was asked by @rgrassian-split on Slack.

@james-milligan
Copy link
Contributor

Changes were made following this issue happy to reopen

@james-milligan
Copy link
Contributor

I will add comments to references to make this change seem more intentional

@beeme1mr
Copy link
Member Author

@rgrassian-split, does this make sense to you? Do you have any concerns?

@rgrassian-split
Copy link
Contributor

@beeme1mr I softly disagree and think the provider having to look up the exact string to fetch the targeting key from the map is just as troublesome as / worse than having to make an additional fetch to attributes before accessing custom attributes. It's just a soft disagree though, overall it's pretty minor and fine with either.

@james-milligan
Copy link
Contributor

One benefit to decoupling the 2 types (context passed by the client, and context passed to a provider) is that we can make changes to the client facing api without requiring all providers to be updated with the new types, although this does nto necessarily have to come in the form of the FlattenedContext, and of course if only relevant if the EvaluationContext type is subject to change

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 a pull request may close this issue.

3 participants