Skip to content

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

Closed
cthoyt opened this issue Apr 30, 2016 · 6 comments
Closed

Confusing context manager #446

cthoyt opened this issue Apr 30, 2016 · 6 comments

Comments

@cthoyt
Copy link

cthoyt commented Apr 30, 2016

On line 799 of connections.py, the __enter__ method returns self.cursor() rather than self.

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.

with pymysql.connect() as conn:
    with conn.cursor() as cur:
        print(cur)

Unfortunately I realize making a change like this might cause a lot of code to break, and must be carefully considered.

@methane
Copy link
Member

methane commented Apr 30, 2016

Unfortunately I realize making a change like this might cause a lot of code to break, and must be carefully considered.

Yes. And current behavior is came from MySQL-python.
There are no choice for now.

@methane methane closed this as completed Apr 30, 2016
@cthoyt
Copy link
Author

cthoyt commented Apr 30, 2016

Thanks for the insight, I appreciate it.

@kostix
Copy link

kostix commented Jul 19, 2016

See also: #396

@evertrol
Copy link

I would think the (correct) behaviour can be achieved by setting a configuration parameter.
This can be either passed as argument to e.g. connect() or Connection(), or set at the package level itself (or even both: then the former should override the latter).

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.

@methane
Copy link
Member

methane commented Aug 16, 2016

There is no correct behavior. DB-API 2.0 doesn't have __enter__ definition.
So you should prefer code without context manager, or you should use convenient wrapper.

I dislike configuration. There are too many options already. Many people asks
questions without reading docstring. When transition, I'll use normal deprecation process.

But problem is not backward compatibility.
Main problem is compatibility with well used but not maintained library: MySQL-python.

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.

@kostix
Copy link

kostix commented Aug 16, 2016

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants