| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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 |
|
|