bpo-31843: sqlite3.connect() now accepts PathLike objects as database name#4299
Conversation
| PyObject* result; | ||
|
|
||
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s|diOiOip", kwlist, | ||
| if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O|diOiOip", kwlist, |
There was a problem hiding this comment.
I don't understand why the function checks arguments.
Why not only relying on Connection constructor to check arguments?
Either remove the whole code to parse arguments, or fix the reference leak: Py_DECREF(database); is needed.
There was a problem hiding this comment.
fix the reference leak: Py_DECREF(database); is needed.
Oops, it's not needed, you don't use PyUnicode_FSConverter, sorry. Your code is correct.
There was a problem hiding this comment.
@Phaqui pointed me on IRC that the function contains a comment explaining that we have to parse all arguments to just extract the "factory" keyword argument... In that case, the code is ok.
| relative to the current working directory) of the database file to be opened. | ||
| You can use ``":memory:"`` to open a database connection to a database that | ||
| resides in RAM instead of on disk. | ||
|
|
There was a problem hiding this comment.
You have to document the change. Something like:
.. versionchanged:: 3.7
*database* can now also be a :term:`path-like object`, not only a string.
| ``":memory:"`` to open a database connection to a database that resides in RAM | ||
| instead of on disk. | ||
| Opens a connection to the SQLite database file *database* and return a | ||
| :class:`Connection` object. |
There was a problem hiding this comment.
"return a Connection" is wrong, since it can be overridden by the factory parameter.
Either remove this addition, or rephrase it like: "Open a connection (...). By default, return a Connection object."
| relative to the current working directory) of the database file to be opened. | ||
| You can use ``":memory:"`` to open a database connection to a database that | ||
| resides in RAM instead of on disk. | ||
|
|
| is PathLike, i.e. has __fspath__(). """ | ||
| self.addCleanup(unlink, TESTFN) | ||
| import pathlib | ||
| path = pathlib.Path(TESTFN) |
There was a problem hiding this comment.
I'm not sure that it's ok to depend on pathlib when testing sqlite.
I propose to replace it with:
class Path(os.PathLike):
def __fspath__(self):
return TESTFN
|
"bedevere/news — No news entry in Misc/NEWS.d/next/" Can you please try to add one using the blurb tool? |
|
All review issues have now been corrected for. |
4 participants
https://bugs.python.org/issue31843