os module: use keyword-only arguments for dir_fd and nofollow to reduce function count

@gvanrossum

BPO	14626
Nosy	@gvanrossum, @birkenfeld, @pitrou, @larryhastings, @benjaminp, @bitdancer, @serhiy-storchaka
Files	larry.os.keyword.arguments.collapse.1.diff larry.os.keyword.arguments.collapse.2.diff larry.os.keyword.arguments.collapse.3.diff: My third patch implementing all this mess. larry.os.keyword.arguments.collapse.4.diff: Fourth patch. larry.os.keyword.arguments.collapse.5.diff: Fifth patch.

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/larryhastings'
closed_at = <Date 2012-06-23.20:41:18.101>
created_at = <Date 2012-04-19.23:52:38.460>
labels = ['type-feature', 'library', 'release-blocker']
title = 'os module: use keyword-only arguments for dir_fd and nofollow to reduce function count'
updated_at = <Date 2015-09-21.23:29:45.974>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2015-09-21.23:29:45.974>
actor = 'python-dev'
assignee = 'larry'
closed = True
closed_date = <Date 2012-06-23.20:41:18.101>
closer = 'georg.brandl'
components = ['Library (Lib)']
creation = <Date 2012-04-19.23:52:38.460>
creator = 'larry'
dependencies = []
files = ['25716', '25818', '25863', '26063', '26080']
hgrepos = []
issue_num = 14626
keywords = ['patch']
message_count = 33.0
messages = ['158777', '158779', '158788', '158795', '158796', '158799', '158801', '158802', '159518', '159626', '159772', '161646', '162271', '162274', '162275', '162514', '162548', '162553', '162554', '162555', '163311', '163393', '163464', '163503', '163506', '163507', '163525', '163576', '163643', '163648', '163669', '251254', '251279']
nosy_count = 12.0
nosy_names = ['gvanrossum', 'georg.brandl', 'pitrou', 'larry', 'benjamin.peterson', 'Arfrever', 'r.david.murray', 'neologix', 'Yury.Selivanov', 'rosslagerwall', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14626'
versions = ['Python 3.3']

Second pass at my patch. Incorporates suggestions from Serhiy's review--thanks, Serhiy!

Still not ready for checkin. > 80 col lines, no docs, docstrings are messy. But code is ready for (further) review. Code passes regression test suite without errors.

Well, I'm going to ignore the long lines and documentation. The patch is
really big and impressive.

I'm not sure that "long" and "impressive" are words that go together when describing a patch ;-)

Here's a nice fresh minor update.

Notes on this third patch:

The docstrings are now done.
I discovered that fchmodat() doesn't actually support
AT_SYMLINK_NOFOLLOW! glibc documents using the flag, then
comically notes that it doesn't actually work. Specifying
it results in ENOTSUP every time. os.chmod() now accomodates
this; it throws NotImplementedError if it detects this situation.
However it should also Just Work once glibc supports it.
While editing the docstrings, I noticed that several of them
had old-style octal constants ;-) I fixed 'em.
I added support for the remove_dir argument to unlink/remove
on Windows. I also made it work on non-Windows even when
unlinkat() is not available.

BTW: If PEP-362 is accepted, and this patch makes it for 3.3 (both of which I think will happen), I'll hand-code signatures for the functions that may throw NotImplementedError so users can use "is_implemented" to LBYL.

I haven't read the code, but from Larry's description this looks great to me. It's amazing how many extra functions were added to the os module since 3.2! I also agree that the redundant functions that existed in 3.2 should stay and I don't see it's fair to deprecate them. I do hope that not too many people have written code based on the 3.3 alphas using all those extra functions, but I suppose they will get what they paid for.

Everything else Larry wrote also sounds reasonable to me.

Previously existing redundant functions could be deprecated in documentation.

Previously existing redundant functions could be
deprecated in documentation.

As in, don't start a "deprecation cycle" (warning in 3.3, deprecated in 3.4, gone in 3.5), just document "consider using this other function instead"? That's probably worth doing. I wouldn't use the word "deprecated" though, I'd just suggest a "see also".

Maybe we could remove the redundant functions in 4.0. I'll put it on my wishlist :)

New patch! What's new:

The big change: I removed the "fd=" parameters. Now, functions that
accept either a path or a file descriptor simply take either as their
"path" argument. I ran it by Guido and he thought it sounded fine,
so I tried it--and I think it's a definite improvement. (Thanks to
Jim Jewett for suggesting it--tbh I'd considered it before, but
looking at it through fresh eyes helped!)
Also new in this patch: you can now LBYL for the fd, dir_fd, and
follow_symlinks parameters. Just check to see if the function is
in os.supports_<name of parameter>. For example:
if os.chown in os.supports_dir_fd:
os.chown(path, dir_fd=whatnot)
The third big bit of news: the patch works under Windows!
I attempted to support Mac OS X 10.3, specifically the weak linking
to statvfs, fstatvfs, and lchown. However I don't have a Mac (much
less one running 10.3) so I can't test this.

I *think* the docstrings are all fixed. The only thing I know that
needs to be done are the docs (and Misc/NEWS).

I really wanna get this in before the feature freeze. I promise to
support it through the betas... can I puh-leez check it in?

Fifth iteration of my patch. Everything is done, and I really think it's ready to be checked in.

The documentation is done, including Misc/NEWS.
All the code is now < 80 columns.
The docstrings have been double- and triple-checked.
It passes all regression tests on Linux 64-bit; it processes the
regression test suite identically to trunk on Windows 32-bit.

> http://bugs.python.org/review/14626/diff/5182/Doc/library/os.rst#newcode1210
Doc/library/os.rst:1210: using it will raise a
:exc:`NotImplementedError`.
Same as above: would it make sense to ignore the arg if follow_symlinks
is not supported, but the fact is irrelevant because the system doesn't
support symlinks at all?

And what if the system supports symlink and doesn't support
at-functions, follow_symlinks=False and file is not symlink. Should it
be NotImplementedError?

New changeset 27f9c26fdd8b by Larry Hastings in branch 'default':
Issue bpo-14626: Large refactoring of functions / parameters in the os module.
http://hg.python.org/cpython/rev/27f9c26fdd8b

New changeset 04fd8f77a58e by Larry Hastings in branch 'default':
Issue bpo-14626: Fix buildbot issues on FreeBSD (AMD64). (Fingers crossed.)
http://hg.python.org/cpython/rev/04fd8f77a58e

New changeset e1e0eeb07398 by Larry Hastings in branch 'default':
Issue bpo-14626: Fix buildbot issue on x86 Tiger 3.x.
http://hg.python.org/cpython/rev/e1e0eeb07398

New changeset 66f7377547d5 by Larry Hastings in branch 'default':
Issue bpo-14626: Fix buildbot issue on OpenIndiana 3.x machines. (Hopefully.)
http://hg.python.org/cpython/rev/66f7377547d5

27f9c26fdd8b broke test_shutil on the Windows buildbots:

======================================================================
FAIL: test_basic (test.test_shutil.TestWhich)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_shutil.py", line 1146, in test_basic
    self.assertEqual(rv, self.temp_file.name)
AssertionError: None != 'c:\\users\\db3l\\appdata\\local\\temp\\tmpxqw4gu\\tmp7ugfmm.exe'

======================================================================
FAIL: test_full_path_short_circuit (test.test_shutil.TestWhich)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_shutil.py", line 1152, in test_full_path_short_circuit
    self.assertEqual(self.temp_file.name, rv)
AssertionError: 'c:\\users\\db3l\\appdata\\local\\temp\\tmpmwer14\\tmpeacfbz.exe' != None

======================================================================
FAIL: test_non_matching_mode (test.test_shutil.TestWhich)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_shutil.py", line 1158, in test_non_matching_mode
    self.assertIsNone(rv)
AssertionError: 'c:\\users\\db3l\\appdata\\local\\temp\\tmp7n6ojp\\tmp5tt9pa.exe' is not None

======================================================================
FAIL: test_pathext_checking (test.test_shutil.TestWhich)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_shutil.py", line 1181, in test_pathext_checking
    self.assertEqual(self.temp_file.name, rv)
AssertionError: 'c:\\users\\db3l\\appdata\\local\\temp\\tmpipmbe3\\tmpx43hex.exe' != None

======================================================================
FAIL: test_relative (test.test_shutil.TestWhich)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_shutil.py", line 1166, in test_relative
    self.assertEqual(rv, os.path.join(tail_dir, self.file))
AssertionError: None != 'tmpcluw7l\\tmp6sy_py.exe'

The rmdir argument to unlink() looks completely crazy to me. Instead rmdir() should be called when one wants to rmdir() a fd.

I agree with Antoine. I've opened bpo-15154 to track this.

The Windows buildbots are now content; closing.

New changeset a138a1131bae by Victor Stinner in branch 'default':
Issue bpo-25207, bpo-14626: Fix ICC compiler warnings in posixmodule.c
https://hg.python.org/cpython/rev/a138a1131bae

New changeset 170cd0104267 by Victor Stinner in branch 'default':
Issue bpo-25207, bpo-14626: Fix my commit.
https://hg.python.org/cpython/rev/170cd0104267

Navigation Menu

Repository navigation

os module: use keyword-only arguments for dir_fd and nofollow to reduce function count #58831

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

os module: use keyword-only arguments for dir_fd and nofollow to reduce function count #58831

Description

Activity

larryhastings commented on Jun 4, 2012

serhiy-storchaka commented on Jun 4, 2012

larryhastings commented on Jun 4, 2012

larryhastings commented on Jun 8, 2012

larryhastings commented on Jun 8, 2012

gvanrossum commented on Jun 9, 2012

Arfrever commented on Jun 9, 2012

larryhastings commented on Jun 9, 2012

larryhastings commented on Jun 21, 2012

larryhastings commented on Jun 22, 2012

serhiy-storchaka commented on Jun 22, 2012

python-dev commented on Jun 22, 2012

python-dev commented on Jun 23, 2012

python-dev commented on Jun 23, 2012

python-dev commented on Jun 23, 2012

pitrou commented on Jun 23, 2012

pitrou commented on Jun 23, 2012

birkenfeld commented on Jun 23, 2012

birkenfeld commented on Jun 23, 2012

python-dev commented on Sep 21, 2015

python-dev commented on Sep 21, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions