bpo-31843: sqlite3.connect() now accepts PathLike objects as database name by Phaqui · Pull Request #4299 · python/cpython

bpo-31843: sqlite3.connect() now accepts PathLike objects as database name by Phaqui · Pull Request #4299 · python/cpython

		PyObject* result;

		if (!PyArg_ParseTupleAndKeywords(args, kwargs, "s\|diOiOip", kwlist,
		if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O\|diOiOip", kwlist,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

		``":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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

		is PathLike, i.e. has __fspath__(). """
		self.addCleanup(unlink, TESTFN)
		import pathlib
		path = pathlib.Path(TESTFN)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

All review issues have now been corrected for.

embray pushed a commit to embray/cpython that referenced this pull request