Merged
Conversation
Contributor
Reviewer's GuideRefactors sp_api.base’s top-level imports to avoid runtime asyncio-related import errors by moving selected imports behind TYPE_CHECKING and implementing lazy, backward-compatible attribute access via getattr for Client and auth-related classes. Sequence diagram for lazy attribute access via getattr in sp_api.basesequenceDiagram
participant Importer
participant sp_api_base as sp_api_base
participant sp_api_base_client as sp_api_base_client
participant sp_api_auth as sp_api_auth
participant sp_api_auth_exceptions as sp_api_auth_exceptions
Importer->>sp_api_base: from sp_api.base import Client
activate sp_api_base
sp_api_base-->>Importer: triggers __getattr__(Client)
sp_api_base->>sp_api_base_client: import Client
sp_api_base_client-->>sp_api_base: Client
sp_api_base-->>Importer: Client (cached in globals)
deactivate sp_api_base
Importer->>sp_api_base: from sp_api.base import AccessTokenClient, Credentials
activate sp_api_base
sp_api_base-->>Importer: triggers __getattr__(AccessTokenClient)
sp_api_base->>sp_api_auth: from sp_api.auth import AccessTokenClient, Credentials
sp_api_auth-->>sp_api_base: AccessTokenClient, Credentials
sp_api_base-->>Importer: AccessTokenClient (cached in globals)
sp_api_base-->>Importer: Credentials (cached in globals)
deactivate sp_api_base
Importer->>sp_api_base: from sp_api.base import AuthorizationError
activate sp_api_base
sp_api_base-->>Importer: triggers __getattr__(AuthorizationError)
sp_api_base->>sp_api_auth_exceptions: from sp_api.auth.exceptions import AuthorizationError
sp_api_auth_exceptions-->>sp_api_base: AuthorizationError
sp_api_base-->>Importer: AuthorizationError (cached in globals)
deactivate sp_api_base
Flow diagram for getattr resolution in sp_api.baseflowchart TD
A(__getattr__ called with name) --> B{Is name Client}
B -->|Yes| C[Import Client from sp_api.base.client]
C --> D[Set globals Client]
D --> E[Return Client]
B -->|No| F{Is name AccessTokenClient or Credentials}
F -->|Yes| G[Import AccessTokenClient and Credentials from sp_api.auth]
G --> H[Select requested export by name]
H --> I[Set globals with selected export]
I --> J[Return selected export]
F -->|No| K{Is name AuthorizationError}
K -->|Yes| L[Import AuthorizationError from sp_api.auth.exceptions]
L --> M[Set globals AuthorizationError]
M --> N[Return AuthorizationError]
K -->|No| O[Raise AttributeError for unknown name]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
__getattr__implementation repeats similar import/assignment logic for each symbol; consider centralizing this into a single mapping dict (name -> import path) to reduce branching and make it easier to extend or modify in the future. - Since these attributes are now lazily imported, it might be safer to derive or validate
__all__from the same set of supported names used in__getattr__to avoid drift if new exports are added or existing ones are renamed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `__getattr__` implementation repeats similar import/assignment logic for each symbol; consider centralizing this into a single mapping dict (name -> import path) to reduce branching and make it easier to extend or modify in the future.
- Since these attributes are now lazily imported, it might be safer to derive or validate `__all__` from the same set of supported names used in `__getattr__` to avoid drift if new exports are added or existing ones are renamed.
## Individual Comments
### Comment 1
<location> `sp_api/base/__init__.py:101` </location>
<code_context>
FulfillmentChannels = FulfillmentChannel
+
+
+def __getattr__(name):
+ if name == "Client":
+ from .client import Client
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the lazy imports into a single mapping of names to loader callables so that `__getattr__` just looks up and caches values instead of handling multiple conditional branches.
You can keep the lazy-import behavior while reducing branching and duplication by centralizing the mapping of exported names to loader functions. This also helps keep `TYPE_CHECKING`, `__all__`, and `__getattr__` in sync.
For example:
```python
from typing import TYPE_CHECKING, Callable, Dict, Any
if TYPE_CHECKING:
from .client import Client
from sp_api.auth import AccessTokenClient, Credentials
from sp_api.auth.exceptions import AuthorizationError
_LAZY_LOADERS: Dict[str, Callable[[], Any]] = {
"Client": lambda: __import__(__name__ + ".client", fromlist=["Client"]).Client,
"AccessTokenClient": lambda: __import__("sp_api.auth", fromlist=["AccessTokenClient"]).AccessTokenClient,
"Credentials": lambda: __import__("sp_api.auth", fromlist=["Credentials"]).Credentials,
"AuthorizationError": lambda: __import__("sp_api.auth.exceptions", fromlist=["AuthorizationError"]).AuthorizationError,
}
def __getattr__(name: str) -> Any:
try:
loader = _LAZY_LOADERS[name]
except KeyError:
raise AttributeError(f"module '{__name__}' has no attribute '{name}'") from None
value = loader()
globals()[name] = value # cache
return value
```
This simplifies:
- One place to list supported lazy symbols (`_LAZY_LOADERS`) instead of multiple `if` branches.
- `__getattr__` no longer mixes unrelated responsibilities; it just looks up a loader and caches.
- Adding/removing a symbol only requires touching `_LAZY_LOADERS`, and optionally `__all__` / `TYPE_CHECKING`, reducing risk of drift.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| FulfillmentChannels = FulfillmentChannel | ||
|
|
||
|
|
||
| def __getattr__(name): |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring the lazy imports into a single mapping of names to loader callables so that __getattr__ just looks up and caches values instead of handling multiple conditional branches.
You can keep the lazy-import behavior while reducing branching and duplication by centralizing the mapping of exported names to loader functions. This also helps keep TYPE_CHECKING, __all__, and __getattr__ in sync.
For example:
from typing import TYPE_CHECKING, Callable, Dict, Any
if TYPE_CHECKING:
from .client import Client
from sp_api.auth import AccessTokenClient, Credentials
from sp_api.auth.exceptions import AuthorizationError
_LAZY_LOADERS: Dict[str, Callable[[], Any]] = {
"Client": lambda: __import__(__name__ + ".client", fromlist=["Client"]).Client,
"AccessTokenClient": lambda: __import__("sp_api.auth", fromlist=["AccessTokenClient"]).AccessTokenClient,
"Credentials": lambda: __import__("sp_api.auth", fromlist=["Credentials"]).Credentials,
"AuthorizationError": lambda: __import__("sp_api.auth.exceptions", fromlist=["AuthorizationError"]).AuthorizationError,
}
def __getattr__(name: str) -> Any:
try:
loader = _LAZY_LOADERS[name]
except KeyError:
raise AttributeError(f"module '{__name__}' has no attribute '{name}'") from None
value = loader()
globals()[name] = value # cache
return valueThis simplifies:
- One place to list supported lazy symbols (
_LAZY_LOADERS) instead of multipleifbranches. __getattr__no longer mixes unrelated responsibilities; it just looks up a loader and caches.- Adding/removing a symbol only requires touching
_LAZY_LOADERS, and optionally__all__/TYPE_CHECKING, reducing risk of drift.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Fix module-level imports in sp_api.base to avoid asyncio-related import errors by lazily loading selected symbols at attribute access time.
Bug Fixes:
Enhancements: