feat: adds evaluation context merging #56
Conversation
Signed-off-by: Manuel Schönlaub <manuel.schoenlaub@gmail.com>
mschoenlaub
force-pushed
the
issue-55
branch
from
335d99e
to
bcabfa5
Compare
|
Some CI is failing, but looking at this from a spec perspective it seems correct! |
There was a problem hiding this comment.
Thanks for taking the time to work towards #55 ❤️ It looks like this needs a bit more work though to be ready to merge 😱
The tests aren't passing due to OpenFeature::SDK.context being nil, so it's hard to validate the rest of the changes.
I'm also not quite sure about using a SimpleDelegator for the Context class. I'm having a hard-time understanding what the actual class is doing.
| @@ -25,7 +25,8 @@ def initialize(provider:, client_options: nil, context: nil) | |||
| # def fetch_boolean_details(flag_key:, default_value:, evaluation_context: nil) | |||
| # result = @provider.fetch_boolean_value(flag_key: flag_key, default_value: default_value, evaluation_context: evaluation_context) | |||
| # end | |||
| def fetch_#{result_type}_#{suffix}(flag_key:, default_value:, evaluation_context: nil) | |||
| def fetch_#{result_type}_#{suffix}(flag_key:, default_value:, evaluation_context: {}) | |||
| evaluation_context = OpenFeature::SDK.context.merge(@context).merge(evaluation_context) | |||
There was a problem hiding this comment.
There are a bunch of CI failures originating back to this line, ie:
NoMethodError:
undefined method `merge' for nil:NilClass
# ./lib/openfeature/sdk/client.rb:29:in `fetch_number_details'
# ./spec/openfeature/sdk/client_spec.rb:150:in `block (6 levels) in <top (required)>'
It doesn't look like OpenFeature::SDK.context ever gets set.
| hashes = values.select { |value| value.is_a?(Hash) } | ||
| while hashes.empty? == false | ||
| hash = hashes.pop | ||
| hash.freeze unless hash.frozen? |
There was a problem hiding this comment.
Note: freeze is perfectly happy to run on a frozen object. Under the hood freeze handles that check for you.
| hash.freeze unless hash.frozen? | |
| hash.freeze |
There was a problem hiding this comment.
Oh yeah, I think what I had in my mind was more a cheap check to avoid running in circles when one of the values contains the hash itself.
While I think there might be no obvious reason to do that, there's nothing in the spec or the language that prohibits that.
.duping stuff could avoid that... but not sure if that's not more expensive in the end.
| # A container for arbitrary contextual data that can be used as a basis for dynamic evaluation | ||
| class Context < SimpleDelegator | ||
|
|
||
| def initialize(context = Concurrent::Hash.new({})) |
There was a problem hiding this comment.
This is part of the concurrent-ruby and it's not a dependency. @josecolella included this dependency initially, but we ended up removing it for simplificy.
There was a problem hiding this comment.
Yeah, I realize that development has suddendly picked up a lot of pace here :D
Given that there seem to be a lot of moving parts right now in this repo, I'll probably revisit this at a later point :)
Hmm.. not sure where exactly concurrent-ruby got removed, but not in this repo. That being said, I'm not unhappy with it being removed at all.
I'm just slightly frustrated with last year's development pace vs. last weeks pace :D
| module SDK | ||
| module Evaluation | ||
| # A container for arbitrary contextual data that can be used as a basis for dynamic evaluation | ||
| class Context < SimpleDelegator |
There was a problem hiding this comment.
I'm not seeing anywhere else in the PR that is using this yet 🤔
| def targeting_key | ||
| self[:targeting_key] | ||
| end | ||
|
|
||
| def targeting_key=(value) | ||
| raise ArgumentError, "targeting_key must be a String" unless value.is_a?(String) | ||
|
|
||
| self[:targeting_key] = value | ||
| end |
There was a problem hiding this comment.
What is targetting key refer to exactly?
There was a problem hiding this comment.
The targeting key is basically a well-defined property in the context, which is necessary because many providers need a user-identifier or equivalent of some kind: https://openfeature.dev/specification/sections/evaluation-context#requirement-311
This PR
Related Issues
Fixes #55
Follow-up Tasks
More tests and better application mocking in the unit tests will be done at a later point.