New driver interface#1252
Conversation
Reviewer's Guide by SourceryThis pull request introduces significant changes to the AsyncDB library, focusing on code restructuring, interface improvements, and the addition of new features. The changes aim to enhance the modularity, readability, and maintainability of the codebase. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @phenobarbital - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
|
||
| class redisPool(BasePool): | ||
| def __init__(self, dsn: str = "", loop: asyncio.AbstractEventLoop = None, params: dict = None, **kwargs) -> None: | ||
| def __init__( |
There was a problem hiding this comment.
issue (complexity): Consider refactoring error handling, simplifying parameter passing, and consolidating similar methods to reduce code complexity.
While the changes provide more specific error handling, which can be beneficial for debugging, the code complexity has increased significantly. Consider the following improvements:
-
Simplify error handling:
Create a helper function to reduce repetition in error handling. For example:def handle_redis_error(error_type, message): if isinstance(error_type, ConnectionError): raise DriverError(f"Error connecting to Redis: {message}") from error_type elif isinstance(error_type, RedisError): raise DriverError(f"Redis error: {message}") from error_type else: raise DriverError(f"Unknown Redis error: {message}") from error_type # Usage in methods: try: # Redis operation except Exception as err: handle_redis_error(err, "Error on operation X")
-
Simplify parameter unpacking:
For methods with many parameters, consider using a configuration object or dataclass. For example:from dataclasses import dataclass @dataclass class RedisConfig: dsn: str = "" loop: asyncio.AbstractEventLoop = None params: dict = None class redisPool(BasePool): def __init__(self, config: RedisConfig, **kwargs): self._dsn = "redis://{host}:{port}/{db}" super(redisPool, self).__init__( dsn=config.dsn, loop=config.loop, params=config.params, **kwargs )
-
Consolidate similar methods:
Methods likekeysandvaluesthat simply call other methods can be removed to reduce code duplication:async def keys(self, key): return await self.hkeys(key) async def values(self, key): return await self.hvals(key)
Instead, use the
hkeysandhvalsmethods directly where needed.
These changes will help reduce complexity while maintaining the improved error handling and functionality.
| args = {} | ||
| if timeout: | ||
| args = {"exptime": timeout} |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Move setting of default value for variable into
elsebranch (introduce-default-else) - Replace if statement with if expression (
assign-if-exp)
| args = {} | |
| if timeout: | |
| args = {"exptime": timeout} | |
| args = {"exptime": timeout} if timeout else {} |
| result = await self._connection.execute_command( | ||
| sentence, | ||
| *args, | ||
| **kwargs | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| try: | ||
| self._loop = asyncio.get_event_loop() | ||
| asyncio.set_event_loop(self._loop) | ||
| except RuntimeError: | ||
| raise RuntimeError( | ||
| f"No Event Loop is running. Please, run this driver inside an asyncio loop." | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace f-string with no interpolated values with string (
remove-redundant-fstring) - Explicitly raise from a previous error (
raise-from-previous-error)
| if not self._loop: | ||
| return True | ||
| if self._loop.is_closed(): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow [×2] (
reintroduce-else) - Replace if statement with if expression [×2] (
assign-if-exp) - Simplify boolean if expression (
boolean-if-exp-identity)
| if not self._loop: | |
| return True | |
| if self._loop.is_closed(): | |
| return True | |
| return False | |
| return True if not self._loop else bool(self._loop.is_closed()) |
| if params: | ||
| return self._dsn.format_map(SafeDict(**params)) | ||
| else: | ||
| return None |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
| if params: | |
| return self._dsn.format_map(SafeDict(**params)) | |
| else: | |
| return None | |
| return self._dsn.format_map(SafeDict(**params)) if params else None |
| if is_missing(value): | ||
| new_val = None | ||
| else: | ||
| new_val = value |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
| if is_missing(value): | |
| new_val = None | |
| else: | |
| new_val = value | |
| new_val = None if is_missing(value) else value |
|
|
||
| def _get_attribute(self, field: Field, value: Any, attr: str = "primary_key") -> Any: | ||
| if hasattr(field, attr): | ||
| datatype = field.type |
There was a problem hiding this comment.
suggestion (code-quality): Move assignments closer to their usage (move-assign)
| datatype = field.type | |
| datatype = field.type |
| result = f"\nWHERE {_and}" | ||
| return result |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| result = f"\nWHERE {_and}" | |
| return result | |
| return f"\nWHERE {_and}" |
|
|
||
| def _get_condition(self, key: str, field: Field, value: Any, datatype: Any) -> str: | ||
| condition = "" | ||
| if isinstance(value, list): |
There was a problem hiding this comment.
issue (code-quality): Lift return into if (lift-return-into-if)
1 participant
Summary by Sourcery
Refactor the driver interface to introduce a new base module, enhancing code organization and readability. Implement a new driver interface with support for asynchronous context management and event loop handling. Improve error handling and add typing annotations for better type safety. Update the library version to 2.9.0.
New Features:
Enhancements:
Chores: