Feat/Global shade module configuration - #53
Conversation
…hread isolation and thread-safe locks for global defaults.
…shade` package namespace (`shade.api_key = "sk_live_..."`).
…ng `api_key` (defaulting to `None`).
…eters via `get_config()` dynamically during `request()`.
…g()` and validate `api_key`.
…cution, instance-level overrides
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe SDK adds thread-safe global configuration exposed through ChangesGlobal configuration and request execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shade/client.py (1)
53-70: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winResolved
timeout/max_retriesare ignored on this path.
get_config()is called withouttimeout/max_retries, and the resolved values are never handed tohttpx, soshade.timeout(and any per-client value) has no effect forShadeClient.request— httpx falls back to its own 5s default, and there is no retry at all. SinceGateway.requestdelegates here, gateway-level settings are silently dropped too.🔧 Proposed fix
- cfg = get_config( - api_key=self.api_key, - api_base=self._base_url, - ) + cfg = get_config( + api_key=self.api_key, + api_base=self._base_url, + ) @@ response = self._http.request( method, url, headers=request_headers, json=json, content=content, + timeout=cfg.timeout, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/client.py` around lines 53 - 70, Update ShadeClient.request to resolve the client or gateway timeout and max_retries through get_config, then pass both resolved values to the underlying self._http.request call. Preserve the existing URL, headers, payload, and debug logging behavior while ensuring request-level settings are no longer silently ignored.
🧹 Nitpick comments (7)
tests/test_global_config.py (2)
122-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a barrier so the isolation test actually interleaves.
Each worker sets and immediately reads its key, so the tasks can (and often will) run to completion sequentially and the test would pass even if config were process-global. A
threading.Barrier(4)between the write and the read forces genuine overlap.💚 Suggested change
+ barrier = threading.Barrier(4) + def worker(thread_id: int, key: str): shade.api_key = key - # Simulate work + barrier.wait(timeout=5) gateway = Gateway() resolved_key = gateway.api_key results[thread_id] = resolved_key🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_global_config.py` around lines 122 - 140, Update test_concurrent_threads_do_not_bleed to create a threading.Barrier for all four workers, have each worker wait at the barrier after assigning shade.api_key and before reading Gateway().api_key, and pass the barrier into each submitted worker so the test forces concurrent interleaving.
96-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the instance
environmentoverride.
test_instance_api_key_beats_globalandtest_instance_api_base_beats_globalcover key/base, but there is no test forGateway(environment="production")whileshade.environment == "sandbox"— which is exactly the path that is currently broken (seesrc/shade/gateway.pyLines 63-81). Adding it would have caught the regression. Want me to draft it?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_global_config.py` around lines 96 - 117, The TestInstanceOverridesBeatsGlobalConfig test class lacks coverage for instance-level environment precedence. Add a test alongside test_instance_api_key_beats_global and test_instance_api_base_beats_global that sets shade.environment to "sandbox", constructs Gateway with environment="production", invokes the existing request flow, and asserts the request uses the production environment rather than the global sandbox value.src/shade/gateway.py (2)
101-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
_base_urlproperty.Nothing in
Gatewayreads it — requests resolve their URL inside the HTTP clients/get_config. It's the only place the instanceenvironmentis honored, which makes the gap above easy to miss. Either delete it or make the request path use it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/gateway.py` around lines 101 - 107, The unused Gateway._base_url property should not remain disconnected from request URL resolution. Remove _base_url and its environment-based fallback, or update the Gateway request path and HTTP client configuration to consistently use it; preserve the intended precedence of _api_base, _config.api_base, and environment.base_url.
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
get_configis imported but unused, and the eager validation is now redundant.Nothing in
Gatewaycallsget_config; the import is dead. Thevalidate_client_settings(...)block also duplicates whatSyncHTTPClient.__init__,AsyncHTTPClient.__init__andget_configalready do — three places to keep in sync. Keeping the constructor-time check for fail-fast ergonomics is fine, but drop the unused import.Also applies to: 57-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/gateway.py` at line 8, Remove the unused get_config import from the gateway module and eliminate the redundant eager validate_client_settings block, while preserving constructor-time validation if it is intentionally retained for fail-fast behavior. Update the relevant Gateway initialization flow without changing the existing client constructors or configuration behavior.src/shade/client.py (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBase URL trimming is inconsistent with
get_config.
config.api_baseis returned untrimmed here while.rstrip("/")binds only to the environment branch;get_configtrims both. Reuse the resolver to avoid two sources of truth.♻️ Suggested change
`@property` def base_url(self) -> str: if self._base_url: return self._base_url - return config.api_base or config.environment.base_url.rstrip("/") + return (config.api_base or config.environment.base_url).rstrip("/")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/client.py` around lines 25 - 29, Update the base_url property to reuse the existing get_config resolver instead of independently selecting config.api_base or config.environment.base_url. Preserve the _base_url override while ensuring the resolved configured URL follows get_config’s trimming behavior.src/shade/config.py (1)
47-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the six identical property/setter pairs into a descriptor.
All six accessors share the exact same body modulo field name; a small descriptor removes ~80 lines and guarantees the thread-local/global logic stays consistent when fields are added.
♻️ Sketch
class _ConfigField: def __init__(self, name, default, parse=None): self._attr = name self._global = f"_global_{name}" self._default = default self._parse = parse def __get__(self, obj, owner=None): if obj is None: return self if hasattr(obj._local, self._attr): return getattr(obj._local, self._attr) with obj._lock: return getattr(obj, self._global) def __set__(self, obj, value): if self._parse is not None: value = self._parse(obj, value) setattr(obj._local, self._attr, value) if threading.current_thread() is threading.main_thread(): with obj._lock: setattr(obj, self._global, value)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/config.py` around lines 47 - 130, Replace the six repetitive accessor pairs in the configuration class with a reusable _ConfigField descriptor implementing the shared thread-local/global get and set behavior. Declare descriptors for api_key, api_base, environment, timeout, max_retries, and debug, passing parse_environment only for environment and preserving each field’s existing defaults and types. Remove the corresponding property and setter methods while keeping main-thread global synchronization unchanged.src/shade/http.py (1)
118-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused helper.
_retry_with_backoffhas no callers; both HTTP clients implement their own retry loops, so delete the helper unless the sync flow is rewritten to use it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shade/http.py` around lines 118 - 133, Remove the unused _retry_with_backoff helper, including its retry loop and related implementation, since no callers use it. Leave the existing retry logic in both HTTP clients unchanged; do not rewrite the sync flow to adopt the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shade/config.py`:
- Around line 54-59: Document on Config and the public shade.* configuration
attributes that assignments made outside the main thread update only
thread-local state and do not change process-wide defaults; direct callers to
perform global setup from the main thread or use an explicit global-setting API
if one is added. Keep the existing thread-isolation behavior unchanged.
- Around line 35-45: Update reset() and the thread-local override handling so a
reset invalidates or clears overrides for all live threads, preventing stale
settings in reused workers; use a registry or reset-generation mechanism
consistent with the existing configuration design. Remove the redundant
hasattr(self._local, "__dict__") check and clear the thread-local state directly
where appropriate, while preserving the default assignments under the lock.
In `@src/shade/gateway.py`:
- Around line 87-89: Update the api_key and environment setters in the Gateway
class so changes propagate to the underlying _http, _async_http, and _client
instances, not only the gateway fields. Reuse the clients’ existing
configuration/update mechanisms where available, and ensure subsequent requests
use the new values.
- Around line 63-81: Propagate the Gateway instance’s environment through the
client construction in the Gateway initializer: pass it to SyncHTTPClient,
AsyncHTTPClient, and ClientShadeClient, and ensure those client classes store it
and provide it to get_config(...) when resolving request configuration. Preserve
explicit api_base behavior while making environment-specific defaults use the
instance value instead of the global config.
In `@src/shade/http.py`:
- Around line 22-23: Update the configuration import in the HTTP module to
explicitly import the shared Config instance exposed by the package, rather than
aliasing the config submodule as _config. Preserve the existing _config.*
accesses and ensure they reference the instance directly without relying on
shade package import order.
---
Outside diff comments:
In `@src/shade/client.py`:
- Around line 53-70: Update ShadeClient.request to resolve the client or gateway
timeout and max_retries through get_config, then pass both resolved values to
the underlying self._http.request call. Preserve the existing URL, headers,
payload, and debug logging behavior while ensuring request-level settings are no
longer silently ignored.
---
Nitpick comments:
In `@src/shade/client.py`:
- Around line 25-29: Update the base_url property to reuse the existing
get_config resolver instead of independently selecting config.api_base or
config.environment.base_url. Preserve the _base_url override while ensuring the
resolved configured URL follows get_config’s trimming behavior.
In `@src/shade/config.py`:
- Around line 47-130: Replace the six repetitive accessor pairs in the
configuration class with a reusable _ConfigField descriptor implementing the
shared thread-local/global get and set behavior. Declare descriptors for
api_key, api_base, environment, timeout, max_retries, and debug, passing
parse_environment only for environment and preserving each field’s existing
defaults and types. Remove the corresponding property and setter methods while
keeping main-thread global synchronization unchanged.
In `@src/shade/gateway.py`:
- Around line 101-107: The unused Gateway._base_url property should not remain
disconnected from request URL resolution. Remove _base_url and its
environment-based fallback, or update the Gateway request path and HTTP client
configuration to consistently use it; preserve the intended precedence of
_api_base, _config.api_base, and environment.base_url.
- Line 8: Remove the unused get_config import from the gateway module and
eliminate the redundant eager validate_client_settings block, while preserving
constructor-time validation if it is intentionally retained for fail-fast
behavior. Update the relevant Gateway initialization flow without changing the
existing client constructors or configuration behavior.
In `@src/shade/http.py`:
- Around line 118-133: Remove the unused _retry_with_backoff helper, including
its retry loop and related implementation, since no callers use it. Leave the
existing retry logic in both HTTP clients unchanged; do not rewrite the sync
flow to adopt the helper.
In `@tests/test_global_config.py`:
- Around line 122-140: Update test_concurrent_threads_do_not_bleed to create a
threading.Barrier for all four workers, have each worker wait at the barrier
after assigning shade.api_key and before reading Gateway().api_key, and pass the
barrier into each submitted worker so the test forces concurrent interleaving.
- Around line 96-117: The TestInstanceOverridesBeatsGlobalConfig test class
lacks coverage for instance-level environment precedence. Add a test alongside
test_instance_api_key_beats_global and test_instance_api_base_beats_global that
sets shade.environment to "sandbox", constructs Gateway with
environment="production", invokes the existing request flow, and asserts the
request uses the production environment rather than the global sandbox value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efdfd4cd-07da-4f21-958b-c87e3215367b
📒 Files selected for processing (7)
src/shade/__init__.pysrc/shade/client.pysrc/shade/config.pysrc/shade/gateway.pysrc/shade/http.pytests/test_client_settings.pytests/test_global_config.py
…y import-order luck
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
LGTM!
Thank you for your contribution.
Reconciles ShadeClient with the thread-local global config merged in ShadeProtocol#53. Resolutions: - config.py: take main's thread-safe Config and get_config wholesale; the api_key global this branch added is already there. Only the missing-key message changes, to name all three ways to supply a key. - client.py: ShadeClient keeps its per-instance role but adopts main's lazy resolution. Explicit arguments are pinned to the instance; omitted ones resolve against the global config per request, so a missing key surfaces as AuthenticationError at request time rather than at construction. Gains main's api_key/environment setters, which propagate to the sub-clients. - gateway.py: Gateway stays a ShadeClient subclass, dropping the constructor and accessors now inherited. Keeps main's positional parameter order. - http.py: take main's dynamic SyncHTTPClient/AsyncHTTPClient; the httpx transport this branch moved out of client.py lands as HTTPXTransport and resolves through get_config like main's version did. - __init__.py: drop the "ShadeClient = Gateway" alias, since ShadeClient is now a real class, and keep main's other exports. Tests asserting construction-time snapshotting are rewritten for the lazy semantics. 353 passed.
2 participants
Description
I have implemented global module-level configuration for the
shadePython SDK (mirroring Stripe Python SDK ergonomics) with thread safety, request-time authentication guards, environment switching, and seamless merging with instance-level overrides.Changes Made
Configuration Core
config.py
Configclass to support thread-safe configuration viathreading.local()for thread isolation and thread-safe locks for global defaults.get_config()helper function to merge instance-level overrides with module-level defaults, compute effective URLs, validate timeout and max_retries ranges, and enforce non-nullapi_key.reset()method to restore config state cleanly during test teardowns.init.py
api_keygetter and setter directly on the top-levelshadepackage namespace (shade.api_key = "sk_live_...").api_keyandget_configin__all__.HTTP Transport & Gateway
gateway.py
Gateway.__init__to allow instantiation without passingapi_key(defaulting toNone).Gatewayto resolve configuration settings viaget_config()at request execution time._base_urlproperty respecting explicitapi_base,shade.api_base, and active environment URL.http.py
SyncHTTPClientandAsyncHTTPClientto resolve parameters viaget_config()dynamically duringrequest().AuthenticationErroris raised at request time whenapi_keyis missing orNone.client.py
ShadeClientrequest()method to evaluateget_config()and validateapi_key.Test Suite
test_global_config.py
shade.api_key = "sk_live_xxx"global assignment and accessibility.shade.environment = "sandbox"/"production"active environment switching.AuthenticationErrorwhenshade.api_key = None.test_client_settings.py
_reset_client_settingsfixture to use_config.reset().All acceptance criteria have been verified and satisfied:
shade.api_key = "sk_live_xxx"sets the global key accessible across all resource calls.shade.environment = "sandbox"switches the active environment.shade.api_key = Noneand then calling any resource raisesAuthenticationErrorwith a clear message.Closes #1
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Ran
python -m pytest:Checklist:
Summary by CodeRabbit
New Features
Bug Fixes