-
Notifications
You must be signed in to change notification settings - Fork 18
FEAT: Adding execute in connection class #189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: jahnvi/global_decimalseperator
Are you sure you want to change the base?
FEAT: Adding execute in connection class #189
Conversation
@@ -185,6 +185,29 @@ def cursor(self) -> Cursor: | |||
cursor = Cursor(self) | |||
self._cursors.add(cursor) # Track the cursor | |||
return cursor | |||
|
|||
def execute(self, sql, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding type annotations to the execute method signature. This will improve code clarity and enable better static analysis tooling support. For example, you might specify the types for sql, args, and the return value (e.g., def execute(self, sql: str, *args: Any) -> Cursor:
).
This helps both readers and tools understand expected usage and can catch certain classes of bugs earlier.
DatabaseError: If there is an error executing the query. | ||
InterfaceError: If the connection is closed. | ||
""" | ||
cursor = self.cursor() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each call to execute
creates a new cursor and adds it to self._cursors
. If these cursors are not explicitly closed or dereferenced, memory usage may accumulate over time, potentially resulting in a memory leak.
There is a need to close the returned cursors, enforcing cursor closure, or providing a context manager to ensure proper cleanup. This will help prevent resource leaks and ensure efficient memory management.
In cursors class we already close\dereference the cursors upon exit.. Please consider documenting it explicityly for future maintainers and code readability.
@@ -185,6 +185,29 @@ def cursor(self) -> Cursor: | |||
cursor = Cursor(self) | |||
self._cursors.add(cursor) # Track the cursor | |||
return cursor | |||
|
|||
def execute(self, sql, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For batch executions or scenarios involving multiple related statements, creating a new cursor for each call to execute may not be the most efficient approach.
Consider providing an option or an additional method that allows reusing a single cursor or enables more explicit cursor management for batch operations. This could improve performance and resource utilization for users who need to execute multiple statements in sequence.
This could be to-do \ enhancement for the next release. Create a backlog for it.
@@ -485,3 +485,164 @@ def test_connection_pooling_basic(conn_str): | |||
|
|||
conn1.close() | |||
conn2.close() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible missing edge case: Please add a test for calling execute after the connection has been closed. This should raise an InterfaceError. Covering this scenario will help ensure robust error handling and proper DB-API compliance.
@@ -485,3 +485,164 @@ def test_connection_pooling_basic(conn_str): | |||
|
|||
conn1.close() | |||
conn2.close() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible missing edge case: Consider adding a test that calls execute with very large parameter sets to check for performance limits and correct handling of large inputs. This can help identify potential scalability or memory issues.
This could be a to-do\future enhancement for overall cursor calls. Create a future backlog item for next semester release
@@ -485,3 +485,164 @@ def test_connection_pooling_basic(conn_str): | |||
|
|||
conn1.close() | |||
conn2.close() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible missing edge case: There is no explicit test for creating multiple simultaneous cursors using execute. Please add a test to verify resource limits and memory usage when many cursors are opened at once.
This could be a to-do\future enhancement for overall cursor calls. Create a future backlog item for next semester release
@@ -485,3 +485,164 @@ def test_connection_pooling_basic(conn_str): | |||
|
|||
conn1.close() | |||
conn2.close() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible missing edge case: Please consider adding a test that explicitly checks whether cursors returned by execute are properly closed and removed from self._cursors. This will help prevent memory/resource leaks and ensure correct cursor lifecycle management.
Work Item / Issue Reference
Summary
This pull request introduces a new convenience method,
execute
, to theConnection
class inmssql_python/connection.py
, allowing users to execute SQL statements directly on the connection without manually creating a cursor. Comprehensive tests have been added to ensure the correctness, error handling, and versatility of this new method, covering a wide range of scenarios including parameter passing, transaction management, and comparison with traditional cursor usage.New feature: Connection-level SQL execution
execute
method to theConnection
class, which creates a new cursor, executes the provided SQL statement (with optional parameters), and returns the cursor. This simplifies executing single SQL statements without explicitly managing cursors.Testing and validation for the new method
tests/test_003_connection.py
to verify the behavior of the newexecute
method, including:connection.execute
andcursor.execute
usage patterns