Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #29983 -- Replaced os.path with pathlib.Path. #10684

Closed
wants to merge 1 commit into from

Conversation

funkybob
Copy link
Contributor

pathlib.Path is more "intuitive" to use, and avoids problems with portability between Windows and not-Windows.

The trouble encountered in #29983 would not have happened if we were using pathlib.

@timgraham timgraham changed the title [Fixes #29983] Replace os.path with pathlib.Path equivalents Fixed #29983 -- Replaced os.path with pathlib.Path. Nov 24, 2018
@timgraham
Copy link
Member

When converting my tutorial I get "TypeError: argument of type 'PosixPath' is not iterable" from

File "/home/tim/code/django/django/db/backends/sqlite3/creation.py", line 12, in is_in_memory_db
    return database_name == ':memory:' or 'mode=memory' in database_name

I guess all the paths needs to use str(BASE_DIR / 'db.sqlite3') or else Django has to become Path aware in all the places where strings were expected.

@funkybob
Copy link
Contributor Author

Interesting... according to the Python docs:
a) support for "path-like object" was only added in 3.7
b) mode=memory and other URI style options are only supported when uri=True is passed to sqlite3.connect

From this I take that we should either expose the uri option, and internally call str on the path when it's set [or require a str, not a path], or stop supporting URI syntax in sqlite DB names.

@timgraham
Copy link
Member

I'm not sure what you mean by "exposing the uri option." That uri stuff dates to 8c99b79 if that helps you understand it better.

I also tested the GIS tutorial changes and got this error django.contrib.gis.gdal.error.GDALException: Invalid data source input type: <class 'pathlib.PosixPath'>.

I think the way to proceed here is to make each documentation change in a separate commit along with a test addition (and a code change, if necessary) -- assuming your chosen solution isn't to wrap each Path in str() (which seems a bit ugly, I guess).

@funkybob
Copy link
Contributor Author

I mean exposing a way to pass uri=True to sqlite3.connect.
https://docs.python.org/3/library/sqlite3.html#sqlite3.connect

Otherwise, from how I read it, we shouldn't be assuming the NAME may be a URI.

@timgraham
Copy link
Member

I'm still not following how that's relevant to the issue but feel free to offer a patch that explains it. With my (perhaps superficial) understanding of the issue, I would pursue the option of making Django coerce Path to str as needed.

@funkybob
Copy link
Contributor Author

The error you showed was caused by:
'mode=memory' in database_name
Which would only make sense if you were passing a URL format for the NAME of the DATABASES setting.

@timgraham
Copy link
Member

You can fix the crash by casting database_name (if it's a Path) to a string before doing that check. There might be other places that expect a string and need to be adjusted.

@imomaliev
Copy link

What is status on this?

@timgraham
Copy link
Member

Closing due to inactivity.

@timgraham timgraham closed this Jun 30, 2019
@treyhunner
Copy link
Member

I'm personally interested in seeing this one go through as I'm trying to standardize on pathlib throughout my Django sites and I'd love to have BASE_DIR use (or at least support) pathlib.Path objects also.

@funkybob's changes already work in Python 3.7 but Python 3.6 and earlier don't support pathlib.Path objects in open and many other places.

Since BASE_DIR isn't used anywhere within Django besides later in the settings file, I believe there are only two lines that need to be changed in Django to continue allowing strings to work while also supporting pathlib.Path objects.

I made those changes here: treyhunner@6f7e782

Instead of simply converting the DATABASE['name'] to a string with str(...) I opted to re-implement os.fspath from Python 3.7. This would allow folks using 3.7 and above to use other objects path-like besides pathlib.Path (anything that supports the os.PathLike interface in Python 3.7 should work).

Should I open this in a new PR? Is there anything else you'd like to see changed before I do so?

@timgraham
Copy link
Member

Of course tests are needed.

@funkybob
Copy link
Contributor Author

funkybob commented Jul 2, 2019

I still maintain the sqlite NAME handling should be fixed, but apparently that passed everyone else by :/

@treyhunner
Copy link
Member

@timgraham would you want tests to demonstrate the sqlite issue and show that it is resolved?

@funkybob I don't understand the URL thing well enough to judge. Though I know that when I fixed that node=memory line there was still one more issue beyond it (in Python 3.6 and below) which was related to SQLite not being able to accept pathlib.Path object.

@timgraham
Copy link
Member

Tests for all changes made, e.g. using a Path in NAME, TEMPLATES 'DIRS', STATICFILES_DIRS (which are changed in the documentation), etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants