Support for with-statement (context manager) for the reader object #28
Conversation
|
Not sure why travis fails. It looks like it ignores my additions? I tested locally with both python2.7 and python3. |
|
This is a good start for the pure Python reader. Travis is failing because the C extension has not been updated. |
|
Ah, that makes sense. I'll see if I can update the C extension too. Brb |
…open to __init__. Changed so that trying to reopen a closed reader throws an exception, similar to get.
|
Tests pass now on Travis. Is it failing because of the lint now? As far as I can see the lint errors are from before my changes. Please review the C changes as it's my first time coding in a C extension for python |
| return NULL; | ||
| } | ||
|
|
||
| Py_INCREF(self); |
There was a problem hiding this comment.
If you are incrementing the reference count here, I think you would need to decrement it in Reader__exit__.
Was this incrementing necessary to prevent errors? Without checking, I am not sure whether the Python interpreter increments the reference count for us or whether this is necessary.
There was a problem hiding this comment.
After some testing, I think the reference counting is correct as you have it.
There was a problem hiding this comment.
I wasn't sure either. I got a segfault when not using Py_INCREF though.
Looking at other peoples code (pysqlite) it looks like they do the same thing:
https://github.com/ghaering/pysqlite/blob/e728ffbcaeb7bfae1d6b7165369bd0ae16cabf95/src/connection.c#1554
|
Thanks! This looks good. I had one comment and question about the reference counting. |
|
Thanks! Merged from the command line and released as 1.3.0. |
I made an attempt at implementing #21. Let me know what you think.