Skip to content

Update os.path module to use file descriptor where possible#1680

Closed
mkurek wants to merge 1 commit into
python:masterfrom
mkurek:os-more-fd
Closed

Update os.path module to use file descriptor where possible#1680
mkurek wants to merge 1 commit into
python:masterfrom
mkurek:os-more-fd

Conversation

@mkurek

@mkurek mkurek commented Oct 20, 2017

Copy link
Copy Markdown

Fix #1653
Use _FdOrPathType when possible in os.path module.

@JelleZijlstra

Copy link
Copy Markdown
Member

(The Travis failure is due to a mypy change. I'll submit a typeshed PR to fix it, but you may end up having to rebase this PR.)

JelleZijlstra added a commit to python/mypy that referenced this pull request Oct 20, 2017
This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.
@mkurek

mkurek commented Oct 20, 2017

Copy link
Copy Markdown
Author

@JelleZijlstra sure, thanks!

gvanrossum pushed a commit to python/mypy that referenced this pull request Oct 20, 2017
This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.
Fix python#1653
Use `_FdOrPathType` when possible in `os.path` module.
@JelleZijlstra

Copy link
Copy Markdown
Member

Fixed now (just needed to restart the build).

@JelleZijlstra

Copy link
Copy Markdown
Member

Is it documented that these accept fds? They're not in os.supports_fd and I don't see anything in https://docs.python.org/3/library/os.path.html#os.scandir.

Looking at the code for os.path.exists (https://github.com/python/cpython/blob/master/Lib/genericpath.py#L16), it seems like it accidentally supports fds because it calls os.stat directly.

@mkurek

mkurek commented Oct 21, 2017

Copy link
Copy Markdown
Author

@JelleZijlstra I wasn't aware of os.supports_fd. So the question is if support for fds for os.path functions is accidentally (and that's why it's undocumented and it could change in the future) or it should be documented. On the other hand https://docs.python.org/3/library/os.path.html#os.path.exists and https://docs.python.org/3/library/os.path.html#os.path.samefile mention using os.stat which is allowing fds.

So what do you suggest? If you like, I could prepare some pull requests to update cpython, either adding these functions to os.supports_fd or documenting that these functions support fds.

@gvanrossum

gvanrossum commented Oct 21, 2017 via email

Copy link
Copy Markdown
Member

@JelleZijlstra

Copy link
Copy Markdown
Member

I agree, so filing a ticket with CPython to clarify whether this behavior is intentional sounds like the right approach.

@mkurek

mkurek commented Oct 26, 2017

Copy link
Copy Markdown
Author

Ok, I've created ticket to CPython to clarify this: https://bugs.python.org/issue31871

JukkaL pushed a commit to python/mypy that referenced this pull request Oct 31, 2017
This is causing typeshed's build to fail (e.g. python/typeshed#1680). Followup from #4130.
@JelleZijlstra

Copy link
Copy Markdown
Member

Serhiy's response on bugs.python.org suggests that the fact that these functions support fd arguments in some platforms is unintentional. Therefore, I'm inclined to not support fd arguments in typeshed.

@gvanrossum

gvanrossum commented Dec 16, 2017 via email

Copy link
Copy Markdown
Member

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.

3 participants