revert: undo config refactor from PR #27 (fixes deadlock) #31
Conversation
WalkthroughThis pull request refactors the ABSmartly Ruby client library by removing Forwardable delegation and replacing it with explicit public attribute accessors across multiple core classes (ABSmartly, ABSmartlyConfig, Client, ClientConfig, Context). The changes introduce public setter methods returning Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/a_b_smartly_config.rb`:
- Around line 21-23: Delete the unused and buggy method context_data_provide:
it’s a dead/typoed method (missing the "r"), returns the wrong instance variable
`@context_event_handler` instead of `@context_data_provider`, and is redundant
because attr_accessor already generates context_data_provider; remove the entire
def context_data_provide ... end block to fix.
In `@lib/absmartly.rb`:
- Line 11: Delete the unused class variable @@init_config from lib/absmartly.rb:
locate the line defining @@init_config and remove that declaration (and any
trailing whitespace), ensuring no other references to @@init_config exist
elsewhere; run a quick grep across the repo to confirm it’s not used and run
tests/lint to validate no regressions.
In `@lib/client.rb`:
- Around line 77-79: The success? method can raise NoMethodError because
`@promise` is only initialized in context_data; update success? (method name
success?) to guard against nil by either invoking context_data to ensure
`@promise` is initialized or returning a safe default when `@promise` is nil (e.g.,
use safe navigation on `@promise` or explicit nil-check and return false), so
calling success? before context_data won’t raise an error.
🧹 Nitpick comments (13)
lib/default_http_client.rb (1)
4-4: Inconsistent string delimiter.This line uses single quotes whilst the rest of the file uses double quotes. Per RuboCop's
Style/StringLiteralsconvention for this project, prefer double quotes for consistency.Suggested fix
-require 'faraday/retry' +require "faraday/retry"example/example.rb (1)
85-85: Missing final newline.Per RuboCop's
Layout/TrailingEmptyLinesconvention, add a newline at the end of the file.Suggested fix
ctx2.close +lib/absmartly.rb (2)
44-51: Instance variable assigned but not memoised.
@client_configis assigned but recreated on every call since there's no memoisation (||=). The instance variable serves no purpose in this pattern. Either remove the instance variable and use a local variable, or memoise if a single config instance is intended.Option A: Use local variable if fresh config is intended
def client_config - `@client_config` = ClientConfig.create - `@client_config.endpoint` = `@endpoint` - `@client_config.api_key` = `@api_key` - `@client_config.application` = `@application` - `@client_config.environment` = `@environment` - `@client_config` + config = ClientConfig.create + config.endpoint = `@endpoint` + config.api_key = `@api_key` + config.application = `@application` + config.environment = `@environment` + config end
53-57: Same non-memoised instance variable pattern.As with
client_config,@sdk_configis assigned but not memoised. Sincesdkis memoised at line 60, this will typically only be called once, but the instance variable assignment is still redundant.lib/a_b_smartly_config.rb (1)
4-6: Consider usingattr_readerinstead ofattr_accessor.Since all setters are manually defined below to return
selffor method chaining, the writer methods generated byattr_accessorare immediately overridden. Usingattr_readerwould make the intent clearer.Suggested change
- attr_accessor :context_data_provider, :context_event_handler, - :variable_parser, :scheduler, :context_event_logger, - :client, :audience_deserializer + attr_reader :context_data_provider, :context_event_handler, + :variable_parser, :scheduler, :context_event_logger, + :client, :audience_deserializerlib/a_b_smartly.rb (2)
47-56: Consider using||=for idiomatic lazy initialisation.The conditional assignments could be simplified using Ruby's idiomatic
||=operator for consistency and brevity.♻️ Proposed refactor
- if `@variable_parser.nil`? - `@variable_parser` = DefaultVariableParser.new - end - - if `@audience_deserializer.nil`? - `@audience_deserializer` = DefaultAudienceDeserializer.new - end - if `@scheduler.nil`? - `@scheduler` = ScheduledThreadPoolExecutor.new(1) - end + `@variable_parser` ||= DefaultVariableParser.new + `@audience_deserializer` ||= DefaultAudienceDeserializer.new + `@scheduler` ||= ScheduledThreadPoolExecutor.new(1)
17-19: Class variable@@init_httpis set but never used.The
configure_clientclass method stores a block in@@init_http(line 18), but the class variable is immediately reset tonilin everyinitializecall (line 26). The stored block is never invoked anywhere in the codebase, making theconfigure_clientmethod dead code.Either remove the
configure_clientmethod entirely, or if HTTP client configuration is intended, restore the logic to actually use the stored block instead of resetting it.Also applies to: 25-26
lib/context.rb (2)
13-13: Redundantattr_reader :datais overridden by explicit method.The
attr_reader :dataon line 13 generates a public getter, but this is immediately overridden by the explicitdatamethod at lines 308-312 which includes thecheck_ready?guard. Theattr_readerfor:datais misleading since it suggests unguarded access.Consider removing
:datafrom theattr_readerdeclaration since you already have the explicit guarded method.♻️ Proposed fix
- attr_reader :data, :pending_count + attr_reader :pending_count
543-545: Minor: Extra blank line at block body end.As flagged by RuboCop, there's an extra empty line before the closing of the block.
♻️ Proposed fix
`@experimentCustomFieldValues`[custom_field_value.name] = value - end - + end endspec/context_spec.rb (2)
739-739: Minor: Missing spaces inside hash literal braces.RuboCop flags the missing spaces inside the hash literal braces for consistency.
♻️ Proposed fix
- data = {"123": 1, "456": 0} + data = { "123": 1, "456": 0 }
760-761: Minor: Extra empty line at block body end.As flagged by RuboCop, there's an extra empty line before the
endof the block.♻️ Proposed fix
expect(context.custom_field_value("exp_test_no_custom_fields", "languages")).to be_nil - endlib/context_config.rb (1)
73-78: Redundantattr_readerdeclaration.Line 78 declares
attr_reader :event_logger, butattr_accessor :event_loggeron line 5 already provides the reader method. Theset_event_loggerfluent setter is a good addition for method chaining consistency, but the redundantattr_readershould be removed.♻️ Suggested fix
def set_event_logger(event_logger) `@event_logger` = event_logger self end - - attr_reader :event_logger endlib/client.rb (1)
12-16: Inconsistent signature betweenself.createandinitialize.
self.createrequiresconfigas a mandatory parameter, butinitializedeclaresconfig = nilas optional. IfClient.newis called directly without a config, it will raise a confusingNoMethodErroronconfig.endpoint(line 17) rather than a clear validation error.Consider making the signature consistent:
♻️ Suggested fix
- def initialize(config = nil, http_client = nil) + def initialize(config, http_client = nil) + raise ArgumentError.new("Missing ClientConfig") if config.nil? + endpoint = config.endpoint
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
example/example.rblib/a_b_smartly.rblib/a_b_smartly_config.rblib/absmartly.rblib/absmartly/version.rblib/client.rblib/client_config.rblib/context.rblib/context_config.rblib/default_http_client.rbspec/a_b_smartly_spec.rbspec/context_spec.rbspec/spec_helper.rb
💤 Files with no reviewable changes (1)
- spec/spec_helper.rb
🧰 Additional context used
🧬 Code graph analysis (6)
example/example.rb (3)
lib/client.rb (1)
close(73-75)lib/context.rb (1)
close(298-306)lib/default_http_client.rb (1)
close(43-45)
lib/client.rb (6)
lib/client_config.rb (7)
attr_accessor(3-43)create(7-9)initialize(21-26)context_data_deserializer(28-30)context_data_deserializer(32-34)context_event_serializer(36-38)context_event_serializer(40-42)lib/default_http_client.rb (6)
attr_accessor(8-62)create(11-13)initialize(15-26)get(31-33)put(35-37)close(43-45)lib/default_context_data_deserializer.rb (2)
attr_accessor(7-16)deserialize(10-15)lib/default_http_client_config.rb (3)
attr_accessor(3-19)create(9-11)initialize(13-18)lib/http_client.rb (4)
get(9-11)response(3-20)response(5-7)put(17-19)lib/default_context_event_serializer.rb (2)
serialize(5-58)serialize(6-57)
lib/absmartly.rb (3)
lib/a_b_smartly.rb (3)
attr_accessor(12-92)configure_client(17-19)create(21-23)lib/a_b_smartly_config.rb (3)
attr_accessor(3-49)create(7-9)client(45-48)lib/client_config.rb (2)
attr_accessor(3-43)create(7-9)
spec/context_spec.rb (1)
lib/context.rb (1)
data(308-312)
lib/a_b_smartly_config.rb (7)
lib/a_b_smartly.rb (2)
attr_accessor(12-92)create(21-23)lib/client_config.rb (2)
attr_accessor(3-43)create(7-9)lib/default_context_event_handler.rb (1)
attr_accessor(5-15)lib/default_audience_deserializer.rb (1)
attr_accessor(5-13)lib/default_variable_parser.rb (1)
attr_accessor(5-13)lib/default_context_data_provider.rb (1)
attr_accessor(5-15)lib/scheduled_thread_pool_executor.rb (1)
attr_accessor(5-14)
lib/context_config.rb (1)
lib/context.rb (1)
attr_reader(12-601)
🪛 RuboCop (1.82.1)
example/example.rb
[convention] 85-85: Final newline missing.
(Layout/TrailingEmptyLines)
lib/default_http_client.rb
[convention] 4-4: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
spec/context_spec.rb
[convention] 739-739: Space inside { missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 739-739: Space inside } missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 760-761: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
lib/context.rb
[convention] 544-545: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (13)
lib/absmartly/version.rb (1)
4-4: LGTM!Version bump to 1.2.2 is appropriate for this bugfix release.
lib/client_config.rb (4)
4-5: LGTM on the public accessors.The addition of
deserializer,serializer, andexecutoras public attributes aligns with the PR's goal of reverting to explicit accessors rather than delegation patterns.
7-9: LGTM on the simplified factory method.The
createmethod now returns a bare instance without parameters, which is appropriate for a builder-style configuration pattern.
11-19: LGTM oncreate_from_properties.The method correctly transforms keys to symbols and assigns the relevant properties using the prefix convention.
28-42: LGTM on the aliased accessors.The
context_data_deserializerandcontext_event_serializeraccessors correctly delegate to the internal@deserializerand@serializerattributes, providing API compatibility.lib/a_b_smartly.rb (3)
34-45: LGTM on client-dependent default provisioning.The logic correctly requires a
Clientinstance whencontext_data_providerorcontext_event_handleris not explicitly provided, and lazily instantiates the default implementations.
59-64: LGTM oncreate_contextwith validation.The method correctly validates parameters before creating the context.
81-91: LGTM on unit validation logic.The
validate_paramsmethod properly validates that unit UIDs are eitherStringorNumerictypes with non-zero length, providing clear error messages.spec/a_b_smartly_spec.rb (1)
24-29: LGTM on updated error expectation.The test correctly reflects the new validation behaviour that requires a
Clientinstance whencontext_data_providerorcontext_event_handlerare not provided.spec/context_spec.rb (1)
1152-1152: UsingSTDOUTwill output debug logs during test runs.Changing from
IO::NULLtoSTDOUTmeans theMockContextEventLoggerProxywill now emit debug output to the console during test execution. This could clutter test output, especially in CI pipelines. Was this intentional for debugging purposes, or should it be reverted to suppress output?lib/client.rb (3)
17-51: LGTM on validation and initialisation logic.The validation provides clear error messages for missing configuration, and the fallback to default deserializer/serializer implementations is sensible. The header and query setup is well-structured.
53-63: LGTM oncontext_datamethod.The method correctly uses instance variables for the HTTP request and deserialisation.
65-71: LGTM onpublishmethod.The method correctly uses instance variables for serialisation and HTTP request.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def context_data_provide | ||
| @context_event_handler | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/a_b_smartly_config.rb | head -50Repository: absmartly/ruby-sdk
Length of output: 1448
🏁 Script executed:
rg -n 'context_data_provide' --type rubyRepository: absmartly/ruby-sdk
Length of output: 2005
🏁 Script executed:
rg -n 'context_data_provider' --type rubyRepository: absmartly/ruby-sdk
Length of output: 1949
Remove dead code method with critical bug.
The method context_data_provide (lines 21–23) is never called anywhere in the codebase and contains two critical errors: the method name is a typo (missing 'r'), and it returns @context_event_handler instead of @context_data_provider. The correct reader context_data_provider is already generated by the attr_accessor on line 4. Delete this method entirely.
🤖 Prompt for AI Agents
In `@lib/a_b_smartly_config.rb` around lines 21 - 23, Delete the unused and buggy
method context_data_provide: it’s a dead/typoed method (missing the "r"),
returns the wrong instance variable `@context_event_handler` instead of
`@context_data_provider`, and is redundant because attr_accessor already generates
context_data_provider; remove the entire def context_data_provide ... end block
to fix.
| require_relative "context_config" | ||
|
|
||
| module Absmartly | ||
| @@init_config = nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '@@init_config' --type rubyRepository: absmartly/ruby-sdk
Length of output: 103
Remove unused class variable @@init_config.
This class variable is defined at line 11 but never referenced anywhere in the codebase. Remove it to eliminate dead code.
🤖 Prompt for AI Agents
In `@lib/absmartly.rb` at line 11, Delete the unused class variable @@init_config
from lib/absmartly.rb: locate the line defining @@init_config and remove that
declaration (and any trailing whitespace), ensuring no other references to
@@init_config exist elsewhere; run a quick grep across the repo to confirm it’s
not used and run tests/lint to validate no regressions.
| def success? | ||
| @promise.success? | ||
| end |
There was a problem hiding this comment.
Potential NoMethodError if success? is called before context_data.
The @promise instance variable is only set when context_data is called. If success? is invoked beforehand, it will raise NoMethodError: undefined method 'success?' for nil:NilClass.
🛠️ Suggested fix with nil guard
def success?
- `@promise.success`?
+ `@promise`&.success? || false
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def success? | |
| @promise.success? | |
| end | |
| def success? | |
| `@promise`&.success? || false | |
| end |
🤖 Prompt for AI Agents
In `@lib/client.rb` around lines 77 - 79, The success? method can raise
NoMethodError because `@promise` is only initialized in context_data; update
success? (method name success?) to guard against nil by either invoking
context_data to ensure `@promise` is initialized or returning a safe default when
`@promise` is nil (e.g., use safe navigation on `@promise` or explicit nil-check and
return false), so calling success? before context_data won’t raise an error.
calthejuggler
force-pushed
the
bugfix/revert-broken-mutex-deadlock-changes
branch
from
f11d3c0 to
4551521
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@example/example.rb`:
- Line 85: The file ends with the call "ctx2.close" but lacks a trailing
newline; update the file so the final line (ctx2.close) is followed by a single
newline character (i.e., ensure the file ends with '\n') to satisfy POSIX
text-file requirements and avoid tooling/diff issues.
In `@lib/a_b_smartly.rb`:
- Around line 66-70: create_context_with is missing the same parameter
validation performed in create_context; call validate_params(config) at the
start of create_context_with (before using config) to ensure invalid unit
configurations are rejected, then proceed to call Context.create(...) just like
create_context does; reference the create_context_with method and the
validate_params(config) utility and ensure the validation happens prior to
constructing the Context and before invoking
AudienceMatcher.new(`@audience_deserializer`).
- Around line 17-19: Remove the dead class variable and method: delete the class
variable @@init_http and the method configure_client from the class (they are
assigned but never used and reset to nil in initialize). Also remove any
references to @@init_http within initialize or elsewhere (if present) so there
are no leftover references; ensure initialize’s behavior is unchanged aside from
dropping the nil assignment. Run tests to confirm no usages remain.
In `@lib/json_expr/expr_evaluator.rb`:
- Line 3: Remove the unused require "bigdecimal" from the top of the file: the
file does not reference BigDecimal anywhere (it uses to_f/to_i), and keeping
that require causes a LoadError since bigdecimal isn't declared as a dependency;
simply delete the require "bigdecimal" line so the code relies on standard
numeric conversions without importing BigDecimal.
♻️ Duplicate comments (3)
lib/absmartly.rb (1)
11-12: Remove unused class variable@@init_config.This class variable is defined but never referenced anywhere. It should be removed to eliminate dead code.
🔧 Suggested fix
module Absmartly - @@init_config = nil - class Error < StandardErrorlib/a_b_smartly_config.rb (1)
21-23: Remove dead code method with typo and incorrect return value.The method
context_data_providehas two critical issues:
- The name is a typo (missing 'r' - should be
context_data_provider)- It returns
@context_event_handlerinstead of@context_data_providerThe correct reader is already generated by
attr_accessoron line 4. Delete this method entirely.🔧 Suggested fix
def context_event_handler=(context_event_handler) `@context_event_handler` = context_event_handler self end - def context_data_provide - `@context_event_handler` - end - def variable_parser=(variable_parser)lib/client.rb (1)
77-79: PotentialNoMethodErrorifsuccess?is called beforecontext_data.The
@promiseinstance variable is only set whencontext_datais called. Ifsuccess?is invoked beforehand, it will raiseNoMethodError: undefined method 'success?' for nil:NilClass.🛠️ Suggested fix with nil guard
def success? - `@promise.success`? + `@promise`&.success? || false end
🧹 Nitpick comments (10)
lib/default_http_client.rb (1)
4-4: Inconsistent string delimiter style.This line uses single quotes whilst the rest of the file uses double quotes. For consistency with the codebase style, consider using double quotes.
Proposed fix
-require 'faraday/retry' +require "faraday/retry"lib/context.rb (2)
13-13: Redundantattr_reader :datadeclaration.The
attr_reader :dataon this line is shadowed by the explicitdatamethod defined at lines 308-312. The explicit method includes acheck_ready?guard, which provides important validation. Theattr_readerdeclaration here is unused and potentially confusing.Consider removing
:datafrom theattr_reader:Proposed fix
- attr_reader :data, :pending_count + attr_reader :pending_count
544-545: Remove extra empty line at block body end.Proposed fix
`@experimentCustomFieldValues`[custom_field_value.name] = value - endspec/context_spec.rb (3)
739-739: Fix hash literal spacing to satisfy style conventions.RuboCop flags missing spaces inside braces. This is a minor formatting issue.
🔧 Suggested fix
- data ={"123": 1, "456": 0} + data = { "123": 1, "456": 0 }
760-761: Remove extra empty line at block body end.RuboCop detects an unnecessary blank line before the closing of the test block.
🔧 Suggested fix
expect(context.custom_field_value("exp_test_no_custom_fields", "languages")).to be_nil - end
1149-1153: Consider usingIO::NULLorStringIOto avoid noisy test output.Logging to
STDOUTduring test runs can produce verbose output that obscures test results. If this was changed for debugging purposes, consider reverting to a null sink or capturing logs in aStringIOfor assertions.🔧 Suggested fix for quieter tests
def initialize `@called` = 0 `@events` = [] - `@logger` = Logger.new(STDOUT) + `@logger` = Logger.new(IO::NULL) endlib/context_config.rb (1)
78-78: Remove redundantattr_readerdeclaration.The
attr_accessor :event_loggeron line 5 already generates both getter and setter methods. This explicitattr_readeris unnecessary and adds confusion.🔧 Suggested fix
def set_event_logger(event_logger) `@event_logger` = event_logger self end - - attr_reader :event_logger endlib/absmartly.rb (1)
44-57: Instance variable assignments without memoisation are confusing.Both
client_configandsdk_configassign to instance variables (@client_config,@sdk_config) but create new instances on every call. This pattern suggests memoisation but doesn't actually memoise. Either apply proper memoisation or remove the instance variable assignments.🔧 Option 1: Remove unnecessary instance variable assignments
def client_config - `@client_config` = ClientConfig.create - `@client_config.endpoint` = `@endpoint` - `@client_config.api_key` = `@api_key` - `@client_config.application` = `@application` - `@client_config.environment` = `@environment` - `@client_config` + config = ClientConfig.create + config.endpoint = `@endpoint` + config.api_key = `@api_key` + config.application = `@application` + config.environment = `@environment` + config end def sdk_config - `@sdk_config` = ABSmartlyConfig.create - `@sdk_config.client` = Client.create(client_config) - `@sdk_config` + config = ABSmartlyConfig.create + config.client = Client.create(client_config) + config end🔧 Option 2: Apply proper memoisation if configs should be reused
def client_config - `@client_config` = ClientConfig.create - `@client_config.endpoint` = `@endpoint` - `@client_config.api_key` = `@api_key` - `@client_config.application` = `@application` - `@client_config.environment` = `@environment` - `@client_config` + `@client_config` ||= begin + config = ClientConfig.create + config.endpoint = `@endpoint` + config.api_key = `@api_key` + config.application = `@application` + config.environment = `@environment` + config + end end def sdk_config - `@sdk_config` = ABSmartlyConfig.create - `@sdk_config.client` = Client.create(client_config) - `@sdk_config` + `@sdk_config` ||= begin + config = ABSmartlyConfig.create + config.client = Client.create(client_config) + config + end endlib/a_b_smartly_config.rb (1)
4-6: Consider usingattr_readerinstead ofattr_accessorto avoid duplicate setter definitions.Since you've defined custom setters that return
selffor fluent chaining, theattr_accessorsetters are being overridden. Usingattr_readerwould make the intent clearer and avoid generating unused setter methods.🔧 Suggested fix
class ABSmartlyConfig - attr_accessor :context_data_provider, :context_event_handler, - :variable_parser, :scheduler, :context_event_logger, - :client, :audience_deserializer + attr_reader :context_data_provider, :context_event_handler, + :variable_parser, :scheduler, :context_event_logger, + :client, :audience_deserializerlib/client_config.rb (1)
28-42: Consider removing redundant accessor methods.The
context_data_deserializerandcontext_event_serializergetter/setter pairs are aliases for thedeserializerandserializerattributes already exposed viaattr_accessor. This duplication could lead to confusion about which accessor to use.If these aliases are needed for backwards compatibility, consider documenting this or adding a deprecation notice.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
example/example.rblib/a_b_smartly.rblib/a_b_smartly_config.rblib/absmartly.rblib/absmartly/version.rblib/client.rblib/client_config.rblib/context.rblib/context_config.rblib/default_http_client.rblib/json_expr/expr_evaluator.rbspec/a_b_smartly_spec.rbspec/context_spec.rbspec/spec_helper.rb
💤 Files with no reviewable changes (1)
- spec/spec_helper.rb
🧰 Additional context used
🧬 Code graph analysis (6)
lib/absmartly.rb (2)
lib/client.rb (2)
attr_accessor(8-80)create(12-14)lib/client_config.rb (2)
attr_accessor(3-43)create(7-9)
lib/client.rb (6)
lib/client_config.rb (7)
attr_accessor(3-43)create(7-9)initialize(21-26)context_data_deserializer(28-30)context_data_deserializer(32-34)context_event_serializer(36-38)context_event_serializer(40-42)lib/default_http_client.rb (6)
attr_accessor(8-62)create(11-13)initialize(15-26)get(31-33)put(35-37)close(43-45)lib/default_context_data_deserializer.rb (2)
attr_accessor(7-16)deserialize(10-15)lib/http_client.rb (4)
get(9-11)response(3-20)response(5-7)put(17-19)lib/context_data_deserializer.rb (2)
deserialize(3-8)deserialize(5-7)lib/default_context_event_serializer.rb (2)
serialize(5-58)serialize(6-57)
lib/client_config.rb (1)
lib/client.rb (3)
attr_accessor(8-80)create(12-14)initialize(16-51)
example/example.rb (3)
lib/client.rb (1)
close(73-75)lib/context.rb (1)
close(298-306)lib/default_http_client.rb (1)
close(43-45)
lib/context.rb (1)
lib/variant_assigner.rb (1)
attr_reader(6-47)
spec/context_spec.rb (1)
lib/context.rb (1)
data(308-312)
🪛 GitHub Actions: CI
lib/json_expr/expr_evaluator.rb
[error] 3-3: LoadError: cannot load such file -- bigdecimal (required by lib/json_expr/expr_evaluator.rb)
🪛 RuboCop (1.82.1)
lib/default_http_client.rb
[convention] 4-4: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
example/example.rb
[convention] 85-85: Final newline missing.
(Layout/TrailingEmptyLines)
lib/context.rb
[convention] 544-545: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
spec/context_spec.rb
[convention] 739-739: Space inside { missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 739-739: Space inside } missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 760-761: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (15)
lib/absmartly/version.rb (1)
4-4: LGTM!Appropriate patch version bump for the deadlock bugfix.
spec/a_b_smartly_spec.rb (1)
24-29: LGTM!The updated error message expectation correctly reflects the reverted validation behaviour where a missing client instance is now the primary validation error.
lib/client.rb (5)
9-10: LGTM!The explicit public accessors and readers provide a clean API surface, replacing the previous Forwardable delegation pattern. This aligns with the revert goal.
12-14: LGTM!The factory method properly creates a Client with a default HTTP client when none is provided.
16-51: LGTM!The initialisation validates all required configuration fields (endpoint, api_key, application, environment) with clear error messages, sets up defaults for deserializer and serializer, and constructs the URL, headers, and query parameters correctly.
53-63: LGTM!The context_data method properly fetches data via HTTP GET, handles errors by setting the exception, and deserializes the response content.
65-71: LGTM!The publish method correctly serializes the event, performs the PUT request, and returns an Exception on failure.
lib/a_b_smartly.rb (5)
6-10: LGTM!The additional requires support the default provider and handler instantiation within this class.
12-15: LGTM!The explicit public accessors provide a cleaner API surface compared to the previous Forwardable delegation approach.
27-56: LGTM!The initialisation logic correctly:
- Extracts configuration from the config object
- Requires a client when context_data_provider or context_event_handler are missing
- Provides sensible defaults for variable_parser, audience_deserializer, and scheduler
59-64: LGTM!The
create_contextmethod validates parameters and correctly wires up dependencies for Context creation.
81-91: LGTM!The validation correctly enforces that unit UIDs must be String or Numeric and non-empty.
lib/client_config.rb (3)
3-5: LGTM!Clean attribute declarations for the configuration properties.
7-9: LGTM!Simple factory method returning a new instance.
11-19: LGTM!The
create_from_propertiesmethod correctly extracts configuration values from a properties hash using the provided prefix.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
example/example.rb
Outdated
| ctx2.track("payment", properties) | ||
|
|
||
| ctx2.close | ||
| ctx2.close |
There was a problem hiding this comment.
Add trailing newline at end of file.
POSIX requires text files to end with a newline character. This avoids issues with some tools and version control diffs.
Proposed fix
ctx2.close
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx2.close | |
| ctx2.close | |
🧰 Tools
🪛 RuboCop (1.82.1)
[convention] 85-85: Final newline missing.
(Layout/TrailingEmptyLines)
🤖 Prompt for AI Agents
In `@example/example.rb` at line 85, The file ends with the call "ctx2.close" but
lacks a trailing newline; update the file so the final line (ctx2.close) is
followed by a single newline character (i.e., ensure the file ends with '\n') to
satisfy POSIX text-file requirements and avoid tooling/diff issues.
| def self.configure_client(&block) | ||
| @@init_http = block | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any usage of @@init_http or init_http across the codebase
rg -n '@@init_http|init_http' --type rubyRepository: absmartly/ruby-sdk
Length of output: 151
Remove unused @@init_http class variable and configure_client method.
The @@init_http class variable is assigned in the configure_client method but is never referenced anywhere in the codebase. It is immediately reset to nil in the initialize method and serves no purpose. Remove the dead code to keep the implementation clean.
Suggested fix
- def self.configure_client(&block)
- @@init_http = block
- end
-
def self.create(config)
ABSmartly.new(config)
end
def initialize(config)
- @@init_http = nil
`@context_data_provider` = config.context_data_provider🤖 Prompt for AI Agents
In `@lib/a_b_smartly.rb` around lines 17 - 19, Remove the dead class variable and
method: delete the class variable @@init_http and the method configure_client
from the class (they are assigned but never used and reset to nil in
initialize). Also remove any references to @@init_http within initialize or
elsewhere (if present) so there are no leftover references; ensure initialize’s
behavior is unchanged aside from dropping the nil assignment. Run tests to
confirm no usages remain.
| def create_context_with(config, data) | ||
| Context.create(get_utc_format, config, data, | ||
| @context_data_provider, @context_event_handler, @context_event_logger, @variable_parser, | ||
| AudienceMatcher.new(@audience_deserializer)) | ||
| end |
There was a problem hiding this comment.
Missing validation in create_context_with.
Unlike create_context, this method does not call validate_params(config). This inconsistency could allow invalid unit configurations to slip through when using pre-fetched data.
🛠️ Suggested fix
def create_context_with(config, data)
+ validate_params(config)
Context.create(get_utc_format, config, data,
`@context_data_provider`, `@context_event_handler`, `@context_event_logger`, `@variable_parser`,
AudienceMatcher.new(`@audience_deserializer`))
end🤖 Prompt for AI Agents
In `@lib/a_b_smartly.rb` around lines 66 - 70, create_context_with is missing the
same parameter validation performed in create_context; call
validate_params(config) at the start of create_context_with (before using
config) to ensure invalid unit configurations are rejected, then proceed to call
Context.create(...) just like create_context does; reference the
create_context_with method and the validate_params(config) utility and ensure
the validation happens prior to constructing the Context and before invoking
AudienceMatcher.new(`@audience_deserializer`).
lib/json_expr/expr_evaluator.rb
Outdated
| @@ -1,5 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require "bigdecimal" | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if bigdecimal is already in Gemfile
echo "=== Checking Gemfile for bigdecimal ==="
if fd -t f "Gemfile" > /dev/null 2>&1; then
fd -t f "Gemfile" --exec cat {}
else
echo "Gemfile not found"
fi
echo ""
echo "=== Checking if BigDecimal is actually used in this file ==="
rg -n "BigDecimal" lib/json_expr/expr_evaluator.rb || echo "BigDecimal not used in this file"
echo ""
echo "=== Full content of lib/json_expr/expr_evaluator.rb to understand context ==="
cat -n lib/json_expr/expr_evaluator.rbRepository: absmartly/ruby-sdk
Length of output: 5859
Remove the unused require "bigdecimal" statement.
The require "bigdecimal" on line 3 is causing a LoadError because bigdecimal is not declared as a dependency in your Gemfile. More importantly, BigDecimal is not used anywhere in this file—the code only uses standard numeric conversions like to_f and to_i. Simply remove this unused require statement.
🧰 Tools
🪛 GitHub Actions: CI
[error] 3-3: LoadError: cannot load such file -- bigdecimal (required by lib/json_expr/expr_evaluator.rb)
🤖 Prompt for AI Agents
In `@lib/json_expr/expr_evaluator.rb` at line 3, Remove the unused require
"bigdecimal" from the top of the file: the file does not reference BigDecimal
anywhere (it uses to_f/to_i), and keeping that require causes a LoadError since
bigdecimal isn't declared as a dependency; simply delete the require
"bigdecimal" line so the code relies on standard numeric conversions without
importing BigDecimal.
calthejuggler
force-pushed
the
bugfix/revert-broken-mutex-deadlock-changes
branch
from
4551521 to
e55c6f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/client.rb`:
- Around line 16-18: The initialize method currently declares config = nil but
immediately calls config.endpoint, which will raise NoMethodError if nil; update
initialize to require a non-nil config (remove the default nil) or add an
explicit nil guard at the start of initialize (e.g., check config.nil? and raise
ArgumentError with a clear message like "Missing configuration") before
accessing config.endpoint so callers get a clear error; refer to the initialize
method in lib/client.rb when making this change.
♻️ Duplicate comments (5)
lib/absmartly.rb (1)
11-12: Remove unused class variable@@init_config.This class variable is defined but never referenced anywhere in the codebase. Remove it to eliminate dead code.
lib/a_b_smartly_config.rb (1)
21-23: Remove dead code method with critical bugs.The method
context_data_provide(lines 21–23) is never called anywhere in the codebase and contains two critical errors: the method name is a typo (missing 'r'), and it returns@context_event_handlerinstead of@context_data_provider. The correct readercontext_data_provideris already generated by theattr_accessoron line 4. Delete this method entirely.lib/client.rb (1)
77-79: PotentialNoMethodErrorifsuccess?is called beforecontext_data.The
@promiseinstance variable is only set whencontext_datais called. Ifsuccess?is invoked beforehand, it will raiseNoMethodError: undefined method 'success?' for nil:NilClass.lib/a_b_smartly.rb (2)
17-19: Remove unused@@init_httpclass variable andconfigure_clientmethod.The
@@init_httpclass variable is assigned in theconfigure_clientmethod but is never referenced anywhere in the codebase. It is immediately reset tonilin theinitializemethod and serves no purpose. Remove the dead code to keep the implementation clean.Also applies to: 26-26
66-70: Missing validation increate_context_with.Unlike
create_context, this method does not callvalidate_params(config). This inconsistency could allow invalid unit configurations to slip through when using pre-fetched data.
🧹 Nitpick comments (9)
lib/default_http_client.rb (1)
3-6: Minor style inconsistency flagged by linter.RuboCop flags line 4 for using single quotes when the project prefers double quotes. The surrounding
requirestatements use double quotes, making this inconsistent.Suggested fix
require "faraday" -require 'faraday/retry' +require "faraday/retry" require "uri"lib/context_config.rb (1)
73-78: Redundantattr_readerdeclaration.The
event_loggerattribute is already declared in theattr_accessoron line 5, which provides both reader and writer methods. Theattr_reader :event_loggeron line 78 is therefore redundant.The
set_event_loggermethod is a good addition for chainable configuration, consistent with other setters in this class.Suggested fix - remove redundant attr_reader
def set_event_logger(event_logger) `@event_logger` = event_logger self end - - attr_reader :event_logger endlib/context.rb (2)
13-13: Potentially confusingattr_reader :datadeclaration.There's an explicit
datamethod defined at lines 308-312 that includes acheck_ready?(true)validation. The explicit method will override theattr_reader, so the validation is preserved. However, having both theattr_readerdeclaration and the explicit method is confusing and theattr_reader :datais effectively dead code.Consider removing
:datafrom theattr_readerto avoid confusion, or documenting why both exist.Suggested fix
- attr_reader :data, :pending_count + attr_reader :pending_count
544-545: Extra blank line at block body end.RuboCop flags an extra empty line at the end of the block body. This is a minor formatting issue.
Suggested fix
`@experimentCustomFieldValues`[custom_field_value.name] = value - endspec/context_spec.rb (3)
739-740: Minor formatting issue flagged by linter.RuboCop flags missing spaces inside the hash literal braces.
Suggested fix
- data = {"123": 1, "456": 0} + data = { "123": 1, "456": 0 }
760-761: Extra blank line at block body end.RuboCop flags an extra empty line before the closing of the
itblock.Suggested fix
expect(context.custom_field_value("exp_test_no_custom_fields", "languages")).to be_nil - end
1149-1153: Test logger now outputs to STDOUT.The
MockContextEventLoggerProxynow logs toSTDOUTinstead of silently discarding output. This will make test runs more verbose. If this was added for debugging the deadlock issue, consider reverting toIO::NULLonce the fix is confirmed stable, to keep test output clean.Suggested fix (if debug logging is no longer needed)
def initialize `@called` = 0 `@events` = [] - `@logger` = Logger.new(STDOUT) + `@logger` = Logger.new(IO::NULL) endlib/absmartly.rb (1)
44-51:client_configcreates a new object on every call.This method instantiates a new
ClientConfigand assigns instance variables to it each time it's called. While currently only invoked indirectly viasdk_config(which is memoised throughsdk), consider memoisingclient_configitself for consistency and to prevent accidental repeated instantiation if called elsewhere.♻️ Suggested memoisation
def client_config - `@client_config` = ClientConfig.create - `@client_config.endpoint` = `@endpoint` - `@client_config.api_key` = `@api_key` - `@client_config.application` = `@application` - `@client_config.environment` = `@environment` - `@client_config` + `@client_config` ||= begin + config = ClientConfig.create + config.endpoint = `@endpoint` + config.api_key = `@api_key` + config.application = `@application` + config.environment = `@environment` + config + end endlib/a_b_smartly_config.rb (1)
4-6: Custom setters returningselfoverrideattr_accessor-generated setters.The
attr_accessoron lines 4–6 generates both getters and setters for all listed attributes. The custom setter methods (lines 11–48) override theattr_accessorsetters to returnselffor method chaining. This works correctly, but the pattern is slightly redundant. If chaining is desired, consider usingattr_readerinstead ofattr_accessorto avoid generating setters that are immediately overridden.♻️ Cleaner approach using attr_reader
- attr_accessor :context_data_provider, :context_event_handler, - :variable_parser, :scheduler, :context_event_logger, - :client, :audience_deserializer + attr_reader :context_data_provider, :context_event_handler, + :variable_parser, :scheduler, :context_event_logger, + :client, :audience_deserializerAlso applies to: 11-48
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
lib/a_b_smartly.rblib/a_b_smartly_config.rblib/absmartly.rblib/absmartly/version.rblib/client.rblib/client_config.rblib/context.rblib/context_config.rblib/default_http_client.rbspec/a_b_smartly_spec.rbspec/context_spec.rbspec/spec_helper.rb
💤 Files with no reviewable changes (1)
- spec/spec_helper.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/absmartly/version.rb
🧰 Additional context used
🧬 Code graph analysis (5)
lib/absmartly.rb (3)
lib/client.rb (2)
attr_accessor(8-80)create(12-14)lib/client_config.rb (2)
attr_accessor(3-43)create(7-9)lib/default_http_client.rb (2)
attr_accessor(8-62)create(11-13)
lib/a_b_smartly_config.rb (7)
lib/a_b_smartly.rb (2)
attr_accessor(12-92)create(21-23)lib/client.rb (2)
attr_accessor(8-80)create(12-14)lib/client_config.rb (2)
attr_accessor(3-43)create(7-9)lib/default_context_event_handler.rb (1)
attr_accessor(5-15)lib/default_audience_deserializer.rb (1)
attr_accessor(5-13)lib/default_variable_parser.rb (1)
attr_accessor(5-13)lib/default_context_data_provider.rb (1)
attr_accessor(5-15)
spec/context_spec.rb (1)
lib/context.rb (1)
data(308-312)
lib/a_b_smartly.rb (5)
lib/default_context_event_handler.rb (2)
attr_accessor(5-15)initialize(8-10)lib/default_audience_deserializer.rb (1)
attr_accessor(5-13)lib/default_variable_parser.rb (1)
attr_accessor(5-13)lib/default_context_data_provider.rb (2)
initialize(8-10)context_data(12-14)lib/scheduled_thread_pool_executor.rb (1)
initialize(8-9)
lib/client_config.rb (1)
lib/client.rb (3)
attr_accessor(8-80)create(12-14)initialize(16-51)
🪛 RuboCop (1.82.1)
spec/context_spec.rb
[convention] 739-739: Space inside { missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 739-739: Space inside } missing.
(Layout/SpaceInsideHashLiteralBraces)
[convention] 760-761: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
lib/context.rb
[convention] 544-545: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
lib/default_http_client.rb
[convention] 4-4: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
(Style/StringLiterals)
🔇 Additional comments (7)
spec/a_b_smartly_spec.rb (1)
24-29: LGTM!The updated error message "Missing Client instance configuration" accurately reflects the validation behaviour after reverting the refactor. The test correctly verifies that
ABSmartly.createraises anArgumentErrorwhen no client is configured.lib/absmartly.rb (1)
59-61: Simple memoisation replacing mutex synchronisation.The
@sdk ||= createpattern removes the previous mutex-based synchronisation that caused deadlocks. This is appropriate for resolving the deadlock issue. Note that||=is not atomic, so in highly concurrent scenarios there's a theoretical race wherecreatecould be called multiple times, but this is generally acceptable for SDK initialisation and avoids the deadlock problem.lib/client.rb (1)
29-50: LGTM: Configuration validation and initialisation.The validation logic correctly checks for required fields (endpoint, api_key, application, environment) and raises informative
ArgumentErrormessages. The header and query setup is properly configured for the ABSmartly API.lib/a_b_smartly.rb (2)
34-56: LGTM: Lazy provisioning of default components.The lazy initialisation logic correctly instantiates default providers only when needed, properly validates client presence when required, and creates appropriate fallback instances for variable_parser, audience_deserializer, and scheduler.
81-91: LGTM: Unit validation logic.The
validate_paramsmethod correctly validates that unit UIDs are either String or Numeric types and have non-empty string representations. Error messages are clear and informative.lib/client_config.rb (2)
3-9: LGTM: SimplifiedClientConfigclass structure.The simplified class with direct attribute accessors and a no-argument factory method is cleaner than the previous delegation-based approach. This aligns with the PR objective of reverting the problematic refactor.
28-42: Intentional accessor aliasing for API compatibility.The
context_data_deserializerandcontext_event_serializermethods provide a semantic API while internally using the generic@deserializerand@serializerinstance variables. This maintains backward compatibility with code expecting the context-specific method names.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def initialize(config = nil, http_client = nil) | ||
| @config = config || ClientConfig.new | ||
| @config.validate! | ||
| endpoint = config.endpoint | ||
| raise ArgumentError.new("Missing Endpoint configuration") if endpoint.nil? || endpoint.empty? |
There was a problem hiding this comment.
Misleading default value for config parameter.
The constructor declares config = nil but immediately calls config.endpoint on line 17, which will raise NoMethodError if config is actually nil. Either remove the default value to make it explicit that config is required, or add a nil guard with a clearer error message.
🐛 Suggested fix: remove misleading default
- def initialize(config = nil, http_client = nil)
+ def initialize(config, http_client = nil)
endpoint = config.endpoint🤖 Prompt for AI Agents
In `@lib/client.rb` around lines 16 - 18, The initialize method currently declares
config = nil but immediately calls config.endpoint, which will raise
NoMethodError if nil; update initialize to require a non-nil config (remove the
default nil) or add an explicit nil guard at the start of initialize (e.g.,
check config.nil? and raise ArgumentError with a clear message like "Missing
configuration") before accessing config.endpoint so callers get a clear error;
refer to the initialize method in lib/client.rb when making this change.
This PR reverts a broken config refactor from PR #27. These changes weren't published until this morning's bugfix in PR #28, which led us to discovering a recursive deadlock lock.
Summary by CodeRabbit
Release Notes v1.2.2
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.