Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
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
Comment on lines +1 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

9 changes: 8 additions & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,11 @@ Style/StringLiteralsInInterpolation:
EnforcedStyle: double_quotes

Layout/LineLength:
Max: 120
Enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to disable it entirely?

Max: 270

Style/AccessorGrouping:
Enabled: false

Metrics/BlockLength:
Enabled: false
8 changes: 8 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ PATH
remote: .
specs:
openfeature-sdk (0.0.1)
sorbet-runtime (~> 0.5.10539)

GEM
remote: https://rubygems.org/
Expand Down Expand Up @@ -42,6 +43,12 @@ GEM
rubocop-ast (1.23.0)
parser (>= 3.1.1.0)
ruby-progressbar (1.11.0)
sorbet (0.5.10539)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

sorbet-static (= 0.5.10539)
sorbet-runtime (0.5.10539)
sorbet-static (0.5.10539-universal-darwin-21)
sorbet-static (0.5.10539-universal-darwin-22)
sorbet-static (0.5.10539-x86_64-linux)
unicode-display_width (2.3.0)

PLATFORMS
Expand All @@ -59,6 +66,7 @@ DEPENDENCIES
rake (~> 13.0)
rspec (~> 3.12.0)
rubocop (~> 1.37.1)
sorbet (~> 0.5.10539)

BUNDLED WITH
2.3.25
59 changes: 57 additions & 2 deletions lib/openfeature/sdk.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,65 @@
# frozen_string_literal: true

require "sorbet-runtime"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just guessing, but is a #typed: true missing here?

require "forwardable"

require_relative "sdk/version"
require_relative "sdk/configuration"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this immense amount of require_relatives? IMHO just require should also work.

require_relative "sdk/client"
require_relative "sdk/metadata"
require_relative "sdk/provider/no_op_provider"

module OpenFeature
# API Initialization and Configuration
#
# Represents the entry point to the SDK, including configuration of <tt>Provider</tt>,<tt>Hook</tt>,
# and building the <tt>Client</tt>
#
# To use the SDK, you can optionally configure a <tt>Provider</tt>, with <tt>Hook</tt>
#
# OpenFeature::SDK.configure do |config|
# config.provider = NoOpProvider.new
# end
#
# If no provider is specified, the <tt>NoOpProvider</tt> is set as the default <tt>Provider</tt>.
# Once the SDK has been configured, a client can be built
#
# client = OpenFeature::SDK.build_client(name: 'my-open-feature-client')
module SDK
class Error < StandardError; end
# Your code goes here...
class << self
extend T::Sig
extend Forwardable

def_delegator :@configuration, :provider
Copy link
Contributor

Choose a reason for hiding this comment

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

I love Ruby for this kind of stuff :)

def_delegator :@configuration, :hooks
def_delegator :@configuration, :context

sig { returns(Configuration) }
def configuration
@configuration ||= T.let(Configuration.new, Configuration)
end

# rubocop:disable Lint/UnusedMethodArgument
sig { params(block: T.proc.params(arg0: Configuration).void).void }
def configure(&block)
return unless block_given?

yield(configuration)
end
# rubocop:enable Lint/UnusedMethodArgument

sig do
params(
name: T.nilable(String),
version: T.nilable(String),
context: T.nilable(EvaluationContext)
).returns(SDK::Client)
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?
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

SDK::Client.new(provider, client_options, context)
end
end
end
end
70 changes: 70 additions & 0 deletions lib/openfeature/sdk/client.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# frozen_string_literal: true
# typed: true

require "sorbet-runtime"
require "forwardable"
require "json"

require_relative "./provider/provider"
require_relative "./evaluation_context"
require_relative "./metadata"
require_relative "./hook/hook"
require_relative "./hook/hook_context"
require_relative "./evaluation_options"
require_relative "./resolver/boolean_resolver"
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a directory for resolver that requires all of these so that outside we could just need one require resolver?

require_relative "./resolver/number_resolver"
require_relative "./resolver/object_resolver"
require_relative "./resolver/string_resolver"

module OpenFeature
module SDK
# TODO: Write
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a leftover. Or it is a real todo, which warrants a more detailed description :)

#
class Client
extend T::Sig
extend Forwardable

class OpenFeatureOptions < T::Struct
const :name, T.nilable(String)
const :version, T.nilable(String)
end

sig { returns(Metadata) }
attr_reader :metadata

sig { returns(T::Array[Hook]) }
attr_accessor :hooks

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
Comment on lines +38 to +48
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


sig do
params(
provider: Provider,
client_options: Metadata,
context: T.nilable(EvaluationContext)
).void
end
def initialize(provider, client_options, context)
@provider = provider
@metadata = client_options.dup.freeze
@context = context.dup.freeze
@hooks = []

@boolean_resolver = Resolver::BooleanResolver.new(provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

I still wonder why we need resolvers if we have providers. Hoping to see the reason soon :)

@number_resolver = Resolver::NumberResolver.new(provider)
@string_resolver = Resolver::StringResolver.new(provider)
@object_resolver = Resolver::ObjectResolver.new(provider)
end
end
end
end
33 changes: 33 additions & 0 deletions lib/openfeature/sdk/configuration.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# frozen_string_literal: true
# typed: true

require "forwardable"

require_relative "./provider/provider"
require_relative "./provider/no_op_provider"

module OpenFeature
module SDK
# TODO: Write documentation
#
class Configuration
extend T::Sig
extend Forwardable

sig { returns(T.nilable(EvaluationContext)) }
attr_accessor :context

sig { returns(SDK::Provider) }
attr_accessor :provider

sig { returns(T::Array[Hook]) }
attr_accessor :hooks

def_delegator :@provider, :metadata

def initialize
@hooks = []
end
end
end
end
15 changes: 15 additions & 0 deletions lib/openfeature/sdk/evaluation_context.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true
# typed: true

# frozen_literal: true

require "sorbet-runtime"
require "date"

class EvaluationContext < T::Struct
CustomFieldValues = T.type_alias { T.any(T::Boolean, String, Integer, Float, T.untyped, DateTime) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why T.untyped?

CustomField = T.type_alias { T::Hash[String, CustomFieldValues] }

const :targeting_key, T.nilable(String)
const :custom_fields, T.nilable(CustomField)
end
9 changes: 9 additions & 0 deletions lib/openfeature/sdk/evaluation_details.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true
# typed: strict

require "sorbet-runtime"
require_relative("./resolution_details")

class EvaluationDetails < ResolutionDetails
const :flag_key, String
end
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

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

See my comment on FeatureFlagEvaluationDetails.

10 changes: 10 additions & 0 deletions lib/openfeature/sdk/evaluation_options.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
# typed: strict

require "sorbet-runtime"
require_relative("./hook/hook")

class EvaluationOptions < T::Struct
const :hooks, T::Array[Hook], default: []
const :hook_hints, T.nilable(T::Hash[String, T.untyped])
end
16 changes: 16 additions & 0 deletions lib/openfeature/sdk/feature_flag_error_code.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true
# typed: true

require "sorbet-runtime"

class FeatureFlagErrorCode < T::Enum
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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
Comment on lines +7 to +15
Copy link
Member

Choose a reason for hiding this comment

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

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

end
12 changes: 12 additions & 0 deletions lib/openfeature/sdk/feature_flag_evaluation_details.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true
# typed: strict

require "sorbet-runtime"
require_relative("./feature_flag_error_code")

class FeatureFlagEvaluationDetails < T::Struct
const :reason, T.nilable(String)
const :variant, T.nilable(String)
const :error_code, T.nilable(FeatureFlagErrorCode)
const :error_message, T.nilable(String)
Comment on lines +8 to +11
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

end
10 changes: 10 additions & 0 deletions lib/openfeature/sdk/flag_evaluation_options.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true
# typed: true

require "sorbet-runtime"
require_relative("./hook")

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

Choose a reason for hiding this comment

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

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

end
44 changes: 44 additions & 0 deletions lib/openfeature/sdk/hook/hook.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true
# typed: true

require "sorbet-runtime"

require_relative("./hook_context")

module Hook
extend T::Sig
extend T::Helpers
interface!

sig do
abstract.params(
hook_context: HookContext,
hook_hints: T.nilable(T::Hash[Symbol, T.untyped])
).returns(EvaluationContext)
end
def before(hook_context:, hook_hints: nil); end

sig do
abstract.params(
hook_context: HookContext,
hook_hints: T.nilable(T::Hash[Symbol, T.untyped])
).returns(EvaluationContext)
end
def after(hook_context:, hook_hints: nil); end

sig do
abstract.params(
hook_context: HookContext,
hook_hints: T.nilable(T::Hash[Symbol, T.untyped])
).returns(EvaluationContext)
end
def error(hook_context:, hook_hints: nil); end

sig do
abstract.params(
hook_context: HookContext,
hook_hints: T.nilable(T::Hash[Symbol, T.untyped])
).returns(EvaluationContext)
end
def finally(hook_context:, hook_hints: nil); end
end
21 changes: 21 additions & 0 deletions lib/openfeature/sdk/hook/hook_context.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# typed: true
# frozen_string_literal: true

require_relative("../metadata")
require_relative("../evaluation_context")

module OpenFeature
module SDK
module Hook
class HookContext < T::Struct
const :flag_key, String
const :default_value, T.any(T::Boolean, String, Integer, Integer, Float)
const :flag_value_type, T.any(String, Integer, Float, TrueClass, FalseClass)
const :context, T.nilable(EvaluationContext)
const :client_metadata, SDK::Metadata
const :provider_metadata, SDK::Metadata
const :logger, T.nilable(T.untyped)
end
end
end
end
Loading