classification
Title: sqlite doc: clarify the scope of the context manager
Type: enhancement Stage: resolved
Components: Documentation Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: berker.peksag Nosy List: berker.peksag, docs@python, ghaering, vigdis, xtreak
Priority: normal Keywords: patch

Created on 2018-09-04 15:16 by vigdis, last changed 2019-05-19 22:36 by berker.peksag. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9079 merged xtreak, 2018-09-06 07:38
PR 13429 merged miss-islington, 2019-05-19 21:52
Messages (14)
msg324591 - (view) Author: Daniel Jakots (vigdis) Date: 2018-09-04 15:16
In my experience, the first encounter for beginners with the context manager is with files. The highlighted feature is that you don't need to close the file, 'with' is going to do it for you.

The sqlite3 documentation talks about the context manager in "12.6.8.3. Using the connection as a context manager". The problem in my opinion is that people may believe that the context manager may manage the open/close which is not the case, reading the Modules/_sqlite/connection.c:pysqlite_connection_exit shows that it only does the commit or the rollback.

I'm not sure about the best fix. It can be either (or both) a sentence in the description and/or adding at then end of the code snippet "con.close()" to show that it still needs to be done.

Thanks!
msg324667 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2018-09-06 07:40
Seems like an extra note is reasonable here. I have created a PR for this. Feedback welcome on the wording.

Thanks.
msg324844 - (view) Author: Daniel Jakots (vigdis) Date: 2018-09-08 14:07
Thanks for working on this bug!

My message focussed on the closing aspect because that was my problem. :)

Maybe the wording should be more general like:
- "Note that this does not automatically call :meth:`close` on the connection object."
+ "Note that this does not automatically handle the connection object."

(I'm not sure if I should comment here or on the PR on gh)
msg324846 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-08 16:23
We always explicitly document what the context manager does even if it simply calls an object's close() method. See https://docs.python.org/3/library/fileinput.html#fileinput.input and https://docs.python.org/3/library/shelve.html#shelve.Shelf for example.

In this case, the documentation already explains what the context manager does after the with block finishes (it also explains the behavior on success and error) with a detailed example.

Would adding a "con.close()" line to the example at https://docs.python.org/3/library/sqlite3.html#using-the-connection-as-a-context-manager clarify the behavior for you?
msg324848 - (view) Author: Daniel Jakots (vigdis) Date: 2018-09-08 17:01
Thanks, I lacked the greater picture.

Yes, adding a "con.close()" line to the snippet would clarify for me!
msg324850 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2018-09-08 17:07
Thanks for the comments. I will add `con.close()` to the examples linked.
Should I remove or change this line as per Daniel's comment "Note that this
does not automatically call :meth:`close` on the connection object."

Thanks
-- 

Regards,
Karthikeyan S
msg324852 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-08 17:21
> Should I remove or change this line as per Daniel's comment "Note that
> this does not automatically call :meth:`close` on the connection object."

I'd prefer to keep the current wording as-is.
msg324854 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2018-09-08 17:32
Thanks, I have pushed a change adding `con.close` at the end of the example and also verified that `./python.exe Doc/includes/sqlite3/ctx_manager.py` works as expected.

Thanks
msg324855 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-08 17:53
PR 9079 looks good to me.

I think we can use this as an opportunity to make the rest of the examples in the sqlite3 documentation more consistent and add missing "conn.close()" calls.
msg324856 - (view) Author: Daniel Jakots (vigdis) Date: 2018-09-08 17:57
Yes, I would totally support this move as it would show users that it's important to close it!
msg324857 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2018-09-08 18:35
Sounds good to me too since I copy paste from there time to time and also an unclosed connection doesn't seem to trigger a ResourceWarning it seems. I have pushed a commit to the same PR. I have changed it manually for all the include files. I have ran the files after the PR with the below and I couldn't see any breakages. Let me know if I have missed anything in the include files. I guess there are some more examples in sqlite3.rst but I don't know if it needs to be added on all snippets since they are more focused on the particular function unlike the files in the include folder that has full programs.

Verified with : 

$ ls -1 Doc/includes/sqlite3/*py > sample.txt
$ cat sample.txt | xargs -n 1 ./python.exe

Thanks
msg324938 - (view) Author: Daniel Jakots (vigdis) Date: 2018-09-10 17:29
LGTM
msg342874 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-05-19 21:52
New changeset 287b84de939db47aa8c6f30734ceb8aba9d1db29 by Berker Peksag (Xtreak) in branch 'master':
bpo-34580: Update sqlite3 examples to call close() explicitly (GH-9079)
https://github.com/python/cpython/commit/287b84de939db47aa8c6f30734ceb8aba9d1db29
msg342877 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-05-19 22:36
New changeset 205c1f0e36e00e6e7cb7fbabaab4f52732859f9e by Berker Peksag (Miss Islington (bot)) in branch '3.7':
bpo-34580: Update sqlite3 examples to call close() explicitly (GH-9079)
https://github.com/python/cpython/commit/205c1f0e36e00e6e7cb7fbabaab4f52732859f9e
History
Date User Action Args
2019-05-19 22:36:52berker.peksagsetmessages: + msg342877
2019-05-19 22:36:50berker.peksagsetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: - Python 3.6
2019-05-19 21:52:39miss-islingtonsetpull_requests: + pull_request13339
2019-05-19 21:52:23berker.peksagsetmessages: + msg342874
2018-09-10 17:29:28vigdissetmessages: + msg324938
2018-09-08 18:35:14xtreaksetmessages: + msg324857
2018-09-08 17:57:46vigdissetmessages: + msg324856
2018-09-08 17:53:42berker.peksagsetmessages: + msg324855
2018-09-08 17:32:52xtreaksetmessages: + msg324854
2018-09-08 17:21:47berker.peksagsetmessages: + msg324852
2018-09-08 17:07:03xtreaksetmessages: + msg324850
2018-09-08 17:01:32vigdissetmessages: + msg324848
2018-09-08 16:23:34berker.peksagsetmessages: + msg324846
versions: + Python 3.6, Python 3.7
2018-09-08 14:07:43vigdissetmessages: + msg324844
2018-09-08 12:34:30serhiy.storchakasetassignee: docs@python -> berker.peksag

nosy: + ghaering, berker.peksag
2018-09-06 07:40:50xtreaksetversions: + Python 3.8
nosy: + docs@python

messages: + msg324667

assignee: docs@python
components: + Documentation
2018-09-06 07:38:19xtreaksetkeywords: + patch
stage: patch review
pull_requests: + pull_request8538
2018-09-06 06:53:18xtreaksetnosy: + xtreak
2018-09-04 15:16:36vigdiscreate