🚧 [OpenFeature Spec] Implementation by josecolella · Pull Request #8 · open-feature/ruby-sdk

josecolella · 2022-11-08T06:15:03Z

⚠️ Best reviewed commit-by-commit

This PR

This PR creates the initial implementation of the OpenFeature specification

Related Issues

References #7

Notes

Follow-up Tasks

How to test

bundle exec rspec

Signed-off-by: Jose Colella <jose.colella@gusto.com>

- Add spec to reflect compliance with https://docs.openfeature.dev/docs/specification/sections/flag-evaluation\#11-api-initialization-and-configuration references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

- Including resolvers that handle logic for individual types references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

Signed-off-by: Jose Colella <jose.colella@gusto.com>

- Add NoOpProvider references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

Signed-off-by: Jose Colella <jose.colella@gusto.com>

lib/openfeature/sdk.rb

toddbaert · 2022-11-09T19:04:28Z

lib/openfeature/sdk/resolver/number_resolver.rb

+    class NumberResolver
+      extend T::Sig
+
+      Number = T.type_alias { T.any(Integer, Float) }


So we alias both floats and ints to this type - is it then possible for application authors to interpret this Number either way?

Both precise ints and fractional numbers / proportions (eg .5) are important for feature flag use cases. As long as an author can interpret the return value to either of these I think this alias is a good idea.

toddbaert · 2022-11-09T19:16:22Z

lib/openfeature/sdk/feature_flag_evaluation_details.rb

+  const :reason, T.nilable(String)
+  const :variant, T.nilable(String)
+  const :error_code, T.nilable(FeatureFlagErrorCode)
+  const :error_message, T.nilable(String)


Is this different than EvaluationDetails? If so, it may be missing a value field.

I wonder whether this was meant to be EvaluationContext . I say this, because it looks like being used as a parameter for the "fetcher" functions.

toddbaert · 2022-11-09T19:16:51Z

lib/openfeature/sdk/evaluation_details.rb

+class EvaluationDetails < ResolutionDetails
+  const :flag_key, String
+end


See my comment on FeatureFlagEvaluationDetails.

toddbaert · 2022-11-09T19:18:36Z

lib/openfeature/sdk/metadata.rb

+    # Within the Metadata structure you have access to the following attribute reader:
+    #
+    # * <tt>name</tt> - Allows you to specify name of the Metadata structure
+    #
+    # * <tt>version</tt> - Allows you to specify version of the Metadata structure


Within the Metadata structure you have access to the following attribute reader...

Nitpick: avoid language like "you" and "I". Instead use passive voice:

Within the Metadata structure, the following attribute readers are available...

toddbaert · 2022-11-09T19:22:25Z

lib/openfeature/sdk/resolution_details.rb

+
+class ResolutionDetails < T::Struct
+  const :value, T.any(String, T::Boolean, Integer, Float, T::Hash[String, T.untyped], T::Array[T.untyped])
+  const :reason, T.nilable(T.any(ResolutionReason, String))


This seems to mean that a reason can be a string, OR a pre-defined enum, which is good. Arbitrary reasons should be allowed.

yep the T.any is equivalent to the Typescript union

toddbaert · 2022-11-09T19:23:19Z

lib/openfeature/sdk/resolution_reason.rb

+  enums do
+    DEFAULT = new("DEFAULT")
+    TARGETING_MATCH = new("TARGETING_MATCH")
+    SPLIT = new("SPLIT")
+    DISABLED = new("DISABLED")
+    UNKNOWN = new("UNKNOWN")
+    ERROR = new("ERROR")
+  end


It may be worth adding in-line doc here if that's possible, you can lift it right from the spec.

toddbaert · 2022-11-09T19:24:32Z

lib/openfeature/sdk/feature_flag_error_code.rb

+  enums do
+    PROVIDER_NOT_READY = new("PROVIDER_NOT_READY")
+    FLAG_NOT_FOUND = new("FLAG_NOT_FOUND")
+    PARSE_ERROR = new("PARSE_ERROR")
+    TYPE_MISMATCH = new("TYPE_MISMATCH")
+    TARGETING_KEY_MISSING = new("TARGETING_KEY_MISSING")
+    INVALID_CONTEXT = new("INVALID_CONTEXT")
+    GENERAL = new("GENERAL")
+  end


It may be worth adding in-line doc here if that's possible, you can lift it right from the spec.

toddbaert · 2022-11-09T19:35:05Z

openfeature-sdk.gemspec

  spec.executables = spec.files.grep(%r{\Aexe/}) { |f| File.basename(f) }
  spec.require_paths = ["lib"]

+  spec.add_dependency "sorbet-runtime", "~> 0.5.10539"


We want to minimize runtime deps, but based on the fact that sorbet seems to be extremely useful and extremely popular, I think this is an acceptable runtime dependency. You may want to specify the version requirement less specifically.

Way up in this PR I actually doubted whether including sorbet is that useful. Ruby comes with RBS in 3.1 onwards. Also, looking at most feature flag SDKs out there they do not seem to bring sorbet either.

The reason I chose sorbet is for type safety at runtime.

In #32 I left out sorbet and just implemented it with it

josecolella · 2022-11-09T21:39:27Z

Gemfile.lock

  remote: .
  specs:
    openfeature-sdk (0.0.1)
+      sorbet-runtime


Making this optional.

technicalpickles

Did a partial commit-by-commit review. Will take another pass in the next few days!

technicalpickles · 2022-11-09T21:59:35Z

spec/openfeature/sdk_spec.rb

+require_relative "../spec_helper"
+
+require_relative "../../lib/openfeature/sdk/provider/no_op_provider"
+require_relative "../../lib/openfeature/sdk/configuration"
+require_relative "../../lib/openfeature/sdk"
+require_relative "../../lib/openfeature/sdk/metadata"


You can save yourself a bunch of requires in specs by moving the require of the main library to spec_helper.rb.

You can also ave yourself having to do ../.. by adding the lib to the RSpec load path. You should be able to add -I lib to .rspec, ie: https://stackoverflow.com/a/43307662/135364

technicalpickles · 2022-11-09T22:02:48Z

spec/openfeature/sdk_spec.rb

+  before do
+    subject
+  end


I'd suggest removing this line, and change the subject sections to be before.

I'm a little hesitant about using subject to do a thing, rather than it being the thing that is being tested. Practically speaking, OpenFeature::SDK would be the real subject, since you are making expectations about it. You could use described_class in that case.

technicalpickles · 2022-11-09T22:04:37Z

spec/openfeature/sdk_spec.rb

+    end
+  end
+
+  context "Requirement 1.1.4" do


Maybe use plain english to describe this instead of the requirement? THis is included in failures.

It'd still be good to reference, so maybe link to it in a comment?

technicalpickles · 2022-11-09T22:06:52Z

lib/openfeature/sdk/client.rb

+      def_delegator :@boolean_resolver, :fetch_value, :fetch_boolean_value
+      def_delegator :@boolean_resolver, :fetch_detailed_value, :fetch_boolean_details
+
+      def_delegator :@number_resolver, :fetch_value, :fetch_number_value
+      def_delegator :@number_resolver, :fetch_detailed_value, :fetch_number_details
+
+      def_delegator :@string_resolver, :fetch_value, :fetch_string_value
+      def_delegator :@string_resolver, :fetch_detailed_value, :fetch_string_details
+
+      def_delegator :@object_resolver, :fetch_value, :fetch_object_value
+      def_delegator :@object_resolver, :fetch_detailed_value, :fetch_object_details


You might be able to leverage method_missing for some of these 🤔 I'm not sure if that would end up being more/less clear and/or code though.

I think delegators are clearer (and probably also more performant) than method_missing.
Also, it might make debugging easier. def_delegator actually creates a method, I don't think that method_missing does that.

In any case I was wondering whether we need this overhead of having different resolvers. I was expecting one resolver with several methods (basically what the spec calls a provider).
Though maybe later in the PR i will find out why we would want the possibility to "Mix And Match".

it might make debugging easier
Implementing respond_to? and respond_to_missing? should mitigate some of this.

I don't think that method_missing does that.

It doesn't own it's own, but can be implemented in a way that defines methods as they go.

technicalpickles · 2022-11-09T22:12:28Z

lib/openfeature/sdk/resolver/boolean_resolver.rb

+  module Resolver
+    # TODO: Write documentation
+    #
+    class BooleanResolver


If there's a lot of duplication between the resolvers, could we use a superclass, module, etc to reduce it? Might also be able to dynamically make a class using Class.new or something.

I expect sorbet may make it tough though.

I'm also wondering if we need the resolvers, or if it could be simpler having something in client?

mschoenlaub

Cool stuff :-)

However, I have to main points for feedback. One relates to using RBS instead of sorbet. Most feature flag SDKs out there don't come with sorbet and RBS is available for Ruby 3.1. It also would be way less clutter in the code.

The other main point relateds to the resolvers. I failed to recognize why we need another layer between providers and the SDK. But I might be missing something there!

mschoenlaub · 2022-11-12T02:26:04Z

.rubocop.yml


 Layout/LineLength:
-  Max: 120
+  Enabled: false


Do we really want to disable it entirely?

mschoenlaub · 2022-11-12T02:32:40Z

Gemfile.lock

    rubocop-ast (1.23.0)
      parser (>= 3.1.1.0)
    ruby-progressbar (1.11.0)
+    sorbet (0.5.10539)


Do we really need sorbet? Ruby 3.1 brings us RBS and sorbet is a an addiitional dependency. I just ask, because OpenFeature SDKs were initially meant to come with a lightweight (e.g. quasi empty) set of dependencies.

Additionally, common practice for gems seems to be to not include a Gemfile.lock in the repo.
Now, I realize that I might be quite oldschool here, and later on in the review I might find out that you actually have renovate or something else updating it, so feel free to ignore my rant :D

Not including a Gemfile would also help resolve the release issue we ran into @josecolella.

lib/openfeature/sdk.rb

mschoenlaub · 2022-11-12T02:34:08Z

lib/openfeature/sdk.rb

+      extend T::Sig
+      extend Forwardable
+
+      def_delegator :@configuration, :provider


I love Ruby for this kind of stuff :)

mschoenlaub · 2022-11-12T02:35:39Z

lib/openfeature/sdk.rb

+      end
+      def build_client(name: nil, version: nil, context: nil)
+        client_options = Metadata.new(name: name, version: version)
+        provider = Provider::NoOpProvider.new if provider.nil?


Do other SDKs do that? It seems fine to me, but it also means that there is now a coupling to a specific provider.

@mschoenlaub With this I'm following the specification point of

The evaluation API allows for the evaluation of feature flag values, independent of any flag control plane or vendor. In the absence of a provider the evaluation API uses the "No-op provider", which simply returns the supplied default flag value.

mschoenlaub · 2022-11-12T03:09:11Z

lib/openfeature/sdk/feature_flag_error_code.rb

+
+require "sorbet-runtime"
+
+class FeatureFlagErrorCode < T::Enum


Do we have other ErrorCodes? If not, we could remove the FeatureFlag prefix. That would improve readability.

mschoenlaub · 2022-11-12T03:12:58Z

lib/openfeature/sdk/feature_flag_evaluation_details.rb

+  const :reason, T.nilable(String)
+  const :variant, T.nilable(String)
+  const :error_code, T.nilable(FeatureFlagErrorCode)
+  const :error_message, T.nilable(String)


I wonder whether this was meant to be EvaluationContext . I say this, because it looks like being used as a parameter for the "fetcher" functions.

mschoenlaub · 2022-11-12T03:14:20Z

lib/openfeature/sdk/flag_evaluation_options.rb

+
+class FlagEvaluationOptions < T::Struct
+  const :hooks, T.nilable(T::Array[Hook])
+  const :hook_hints, T.nilable(T::Hash[String, T.untyped])


Hints do not support arbitrary types. Only boolean | string | number | datetime | structure according to spec.

mschoenlaub · 2022-11-12T03:22:13Z

lib/openfeature/sdk/resolver/object_resolver.rb

+      private
+
+      def correct_type?(value)
+        result = JSON.parse(value)


I noticed that the spec says Structured data, presented however is idiomatic in the implementation language, such as JSON or YAML.. However, I would argue that the idiomatic representation in Ruby would be a Hash.
After all, neitehr JSON nor YAML should be forced upon the user of the SDK IMHO.

mschoenlaub · 2022-11-12T03:25:09Z

openfeature-sdk.gemspec

  spec.executables = spec.files.grep(%r{\Aexe/}) { |f| File.basename(f) }
  spec.require_paths = ["lib"]

+  spec.add_dependency "sorbet-runtime", "~> 0.5.10539"


Way up in this PR I actually doubted whether including sorbet is that useful. Ruby comes with RBS in 3.1 onwards. Also, looking at most feature flag SDKs out there they do not seem to bring sorbet either.

technicalpickles · 2022-11-13T20:06:38Z

.editorconfig

+root = true
+
+[*]
+end_of_line = lf
+indent_size = 4
+indent_style = space
+insert_final_newline = true
+tab_width = 8
+trim_trailing_whitespace = true
+
+[*.bat]
+end_of_line = crlf
+
+[*.gemspec]
+indent_size = 2
+
+[*.rb]
+indent_size = 2
+
+[*.yml]
+indent_size = 2
+
+[{*[Mm]akefile*,*.mak,*.mk,depend}]
+indent_style = tab
+
+[enc/*]
+indent_size = 2
+
+[reg*.[ch]]
+indent_size = 2


Let's pull this and the Rubocop rules out as a separate PR.

josecolella changed the base branch from main to feat/set-up-project-structure November 8, 2022 06:15

josecolella force-pushed the feat/spec-implementation branch from bd46bc5 to 89a428b Compare November 8, 2022 06:15

josecolella closed this Nov 8, 2022

josecolella reopened this Nov 8, 2022

Base automatically changed from feat/set-up-project-structure to main November 8, 2022 15:31

beeme1mr requested a review from toddbaert November 8, 2022 17:10

josecolella added 9 commits November 9, 2022 10:43

build: Add sorbet and sorbet-runtime

c62a282

Signed-off-by: Jose Colella <jose.colella@gusto.com>

feat: Implement API initialization and configuration

90962fa

- Add spec to reflect compliance with https://docs.openfeature.dev/docs/specification/sections/flag-evaluation\#11-api-initialization-and-configuration references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

feat: Implement Client

ca58dca

- Including resolvers that handle logic for individual types references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

style: Add .editorconfig

29438a9

Signed-off-by: Jose Colella <jose.colella@gusto.com>

feat: Implement Provider interface

51d6d38

- Add NoOpProvider references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

feat: Add Metadata structure

66d80cc

references #7 Signed-off-by: Jose Colella <jose.colella@gusto.com>

style: Update rubocop rules

cb92584

Signed-off-by: Jose Colella <jose.colella@gusto.com>

build: Update Gemfile.lock

9a4f22a

Signed-off-by: Jose Colella <jose.colella@gusto.com>

feat: Add types

96cc4c1

Signed-off-by: Jose Colella <jose.colella@gusto.com>

josecolella force-pushed the feat/spec-implementation branch from 89a428b to 96cc4c1 Compare November 9, 2022 18:44

josecolella commented Nov 9, 2022

View reviewed changes

lib/openfeature/sdk.rb Show resolved Hide resolved

toddbaert reviewed Nov 9, 2022

View reviewed changes

josecolella commented Nov 9, 2022

View reviewed changes

Gemfile.lock Outdated

remote: .

specs:

openfeature-sdk (0.0.1)

sorbet-runtime

Copy link

Collaborator Author

josecolella Nov 9, 2022

Choose a reason for hiding this comment

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

Making this optional.

technicalpickles reviewed Nov 9, 2022

View reviewed changes

mschoenlaub reviewed Nov 12, 2022

View reviewed changes

technicalpickles reviewed Nov 13, 2022

View reviewed changes

josecolella closed this Nov 14, 2022

technicalpickles mentioned this pull request Sep 12, 2023

chore: add type signatures and type checker for CI #58

Closed


		require "sorbet-runtime"

		class FeatureFlagErrorCode < T::Enum

Conversation

josecolella commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR

Related Issues

Notes

Follow-up Tasks

How to test

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicalpickles left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mschoenlaub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josecolella commented Nov 8, 2022 •

edited

Loading