Skip to content

Conversation

@calthejuggler
Copy link
Contributor

@calthejuggler calthejuggler commented Jan 14, 2026

This PR addresses some review comments from PR #31. As that PR is a simple reversion - I have put these here for ease of reviewing.

Summary by CodeRabbit

  • Bug Fixes

    • Enforced parameter validation during context creation.
    • Improved resilience when checking client operation status to avoid errors if internal state is absent.
  • Breaking Changes

    • Client initialisation now requires a configuration parameter.
    • Removed deprecated public configuration/context-access methods and cleaned up unused internal configuration variables.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

Caution

Review failed

The pull request is closed.

Walkthrough

The PR removes unused public API and class variables: ABSmartly.configure_client and its associated initialization use, ABSmartlyConfig#context_data_provide, and @@init_config from the Absmartly module. ABSmartly#initialize no longer resets @@init_http. ABSmartly#create_context_with(config, data) now calls validate_params(config) before creating a Context. Client#initialize was changed to require config, and Client#success? now safely handles a nil @promise.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐇 I hopped through code at break of day,
Tucked unused bits and tucked them away.
Configs now firm, params checked with care,
Fewer surprises, lighter the air.
A rabbit's tidy patch — hop, skip, hooray! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: address coderabbit comments from #31' is directly related to the PR's objective of addressing review comments from PR #31, and accurately summarises the primary purpose of the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5739887 and 5ae5238.

📒 Files selected for processing (4)
  • lib/a_b_smartly.rb
  • lib/a_b_smartly_config.rb
  • lib/absmartly.rb
  • lib/client.rb

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@calthejuggler
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Jan 14, 2026

✅ Actions performed

Full review triggered.

Copy link

@cwmpinho cwmpinho left a comment

Choose a reason for hiding this comment

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

LGTM!
Tested locally that exposure event is registered correctly

Base automatically changed from bugfix/revert-broken-mutex-deadlock-changes to main January 15, 2026 12:43
@calthejuggler calthejuggler force-pushed the bugfix/coderabbit-comments branch from 5739887 to 5ae5238 Compare January 15, 2026 12:43
@calthejuggler calthejuggler merged commit 74e8e4d into main Jan 15, 2026
6 of 7 checks passed
@calthejuggler calthejuggler deleted the bugfix/coderabbit-comments branch January 15, 2026 12:45
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 this pull request may close these issues.

4 participants