[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Change by Berker Peksag :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions:  -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Berker Peksag  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[miss-islington]

Change by miss-islington :


--
pull_requests: +13339

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Berker Peksag  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Daniel Jakots]

Daniel Jakots  added the comment:

LGTM

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Daniel Jakots]

Daniel Jakots  added the comment:

Yes, I would totally support this move as it would show users that it's 
important to close it!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Berker Peksag  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Berker Peksag  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Daniel Jakots]

Daniel Jakots  added the comment:

Thanks, I lacked the greater picture.

Yes, adding a "con.close()" line to the snippet would clarify for me!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Berker Peksag]

Berker Peksag  added the comment:

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?

--
versions: +Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Daniel Jakots]

Daniel Jakots  added the comment:

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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
assignee: docs@python -> berker.peksag
nosy: +berker.peksag, ghaering

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

Seems like an extra note is reasonable here. I have created a PR for this. 
Feedback welcome on the wording.

Thanks.

--
assignee:  -> docs@python
components: +Documentation
nosy: +docs@python
versions: +Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
keywords: +patch
pull_requests: +8538
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +xtreak

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34580] sqlite doc: clarify the scope of the context manager

[Daniel Jakots]

New submission from Daniel Jakots :

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!

--
messages: 324591
nosy: vigdis
priority: normal
severity: normal
status: open
title: sqlite doc: clarify the scope of the context manager
type: enhancement

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com