Skip to content

Remove context interface from Connector #295

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

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Remove context interface from Connector #295

merged 1 commit into from
Dec 4, 2018

Conversation

methane
Copy link
Member

@methane methane commented Dec 4, 2018

No description provided.

@methane methane merged commit c64915b into PyMySQL:next Dec 4, 2018
@methane methane deleted the remove-context branch December 4, 2018 11:53
@antimirov
Copy link

Guys, sorry for writing into this closed ticket, but what was the reasoning behind the removal of such an important functionality?

@methane
Copy link
Member Author

methane commented Dec 9, 2019

What do you expect the context interface do?
Do you read the code? The removed code did what you expected?

@antimirov
Copy link

antimirov commented Dec 9, 2019

Short background: I've always used Postgresql, mostly with psycopg2, which has similar contextmanager functionality.

My confusion was that while in this pull request #295 (Dec 2018) you deleted enter and exit methods of the connection, in the recent (10 days ago) PR #413 it's back again, albeit in a much shorter form. That's why I was confused, what was the reason to remove it and why is it now okay to put it back?

@methane
Copy link
Member Author

methane commented Dec 9, 2019

What do you expect the context interface do?
Do you read the code? The removed code did what you expected?

@antimirov
Copy link

From what I can see the old code was quite useful - it provided a contextmanager which was creating a cursor instance, plus doing a commit or rollback in exit.

To answer your questions: This is what I would expect from such a ctx manager. Yes, I read the code. Yes, it seems like it did what I expected.

@methane
Copy link
Member Author

methane commented Dec 10, 2019

Hm, it seems psycopg2 behavior is the same to old __exit__ in mysqlclient.
But it wasn't many users expect. Many people want __exit__ close the connection.

That's why I change the behavior of the __exit__.

And I removed the __exit__ once and add it again with different behavior because I want to notice every user. Many users don't care about DeprecationWarning.

@antimirov
Copy link

I agree, with some of the comments in your links that I would like a contextmanager of a connection to returns a connection, and for cursor return a cursor, not to combine these things in one entity. So I can have extra control when I want to apply .commit() manually, for example.

@kate-shine
Copy link

While I agree that your changes make sense, you are making the behavior of mysqlclient-python inconsistent with not only psycopg2, but with the standard library sqlite3 as well.

https://docs.python.org/3.8/library/sqlite3.html#using-the-connection-as-a-context-manager

Some people probably expected a different behavior, but are you sure that there aren't more of them who expect it to behave the way the rest of the world does?

Silviu-Surcica pushed a commit to SpamExperts/mysqlclient-python that referenced this pull request May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants