-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Confusing context manager #446
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
Comments
Yes. And current behavior is came from MySQL-python. |
Thanks for the insight, I appreciate it. |
See also: #396 |
I would think the (correct) behaviour can be achieved by setting a configuration parameter. While backwards compatibility is something worth, in this case it is really unexpected, and the behaviour should at some point in the future be changed. Using a configuration parameter can ease that transition. |
There is no correct behavior. DB-API 2.0 doesn't have I dislike configuration. There are too many options already. Many people asks But problem is not backward compatibility. See also: PyMySQL/mysqlclient#44 and https://sourceforge.net/p/pypi/support-requests/553/ After it solved, I'll think about backward incompatible API changes with normal deprecation process. |
I concur. The behaviour of the context manager was surprising to me but what it needs is better documentation, not another configuration knob. I solved that for me with a custom (and very simple) wrapper which explicitly implements whatever policy I wanted it to. Proliferation of configuration knobs is IMO a bad thing: it explodes the number of cases to test and makes code harder to read. |
On line 799 of connections.py, the
__enter__
method returnsself.cursor()
rather thanself
.While this isn't a bug, I think it makes more sense to return
self
so as to adhere to the example below, which is a common pattern shared by most python connectors. This also allows more user flexibility while still taking advantage of the power of context managers.Unfortunately I realize making a change like this might cause a lot of code to break, and must be carefully considered.
The text was updated successfully, but these errors were encountered: