-
Notifications
You must be signed in to change notification settings - Fork 92
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
feat: Implementing client side statements in dbapi (starting with commit) #1037
Conversation
eec98bf
to
bedb240
Compare
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
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.
Thanks for putting this together! No major concerns but a bunch of little comments. Let me know what you think.
Thanks @aseering for taking out time and providing valuable comments. Got to learn a lot from your comments |
@@ -174,12 +177,11 @@ | |||
RE_PYFORMAT = re.compile(r"(%s|%\([^\(\)]+\)s)+", re.DOTALL) | |||
|
|||
|
|||
@deprecated(reason="This method is deprecated. Use _classify_stmt method") |
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.
this pulls in two new dependencies (deprecated, wrapt) into this library -- this can be replaced entirely with this inside the function body:
warnings.warn("This method is deprecated. Use classify_statement method instead", stacklevel=2)
this uses warnings
directly avoiding the two new dependencies (including one which has a C extension!)
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.
would one be open to a PR to do this instead?
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.
@ankiaga Would you mind taking a look what would be the best solution here?
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.
threw one together: #1120
Implementation to Commit the ongoing transaction whenever execute() method of Cursor is called with a sql query "COMMIT" (or "COMMIT TRANSACTION")
The plan is to later add more client side statements