8000 bpo-1154351: Add get_current_dir_name() to os module by bradengroom · Pull Request #10117 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content
10000

bpo-1154351: Add get_current_dir_name() to os module #10117

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

bpo-1154351: Add get_current_dir_name() to os module #10117

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1daae93
bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 26, 2018
ee4c4f6
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 26, 2018
f836691
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 26, 2018
89e1984
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 26, 2018
4bfb33c
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 27, 2018
963659a
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 27, 2018
cc1735f
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 27, 2018
d49fdf3
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 27, 2018
17386e1
fixup! bpo-1154351: add get_current_dir_name() to os module
bradengroom Oct 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
bpo-1154351: add get_current_dir_name() to os module 
Co-authored-by: Marc Adam Anderson <marc@marcadam.com> .
  • Loading branch information
@bradengroom
bradengroom committed Oct 26, 2018
commit 1daae937be599e43e6b5af0104904c92be249cdd
14 changes: 14 additions & 0 deletions Doc/library/os.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ process and user.
.. function:: chdir(path)
fchdir(fd)
getcwd()
get_current_dir_name()
:noindex:

These functions are described in :ref:`os-file-dir`.
Expand Down Expand Up @@ -1698,6 +1699,19 @@ features:
Return a bytestring representing the current working directory.


.. function:: get_current_dir_name()

Return a string representing the current working directory taking into
consideration the users ``PWD`` environment variable if it exists. This is
Copy link
Member

Choose a reason for hiding this comment

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

Use :envvar:?

opposed to :func:`getcwd()` which dereferences symlinks in the path. This
Copy link
Member

Choose a reason for hiding this comment

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

Remove (). It is added automatically.

function is identical to :func:`getcwd()` on systems that do **not**
support the ``PWD`` environment variable.

.. availability:: Unix.
Copy link
Member

Choose a reason for hiding this comment

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

This function is available on all platforms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, while this is undoubtably an ancient link ( https://www.gnu.org/software/gnulib/manual/html_node/get_005fcurrent_005fdir_005fname.html ) I do not believe this function is available in all POSIX environments. It may be common in GNU runtime environments.

FYI: I just checked AIX and found the routine name "exists" in AIX <unistd.h>, but there is no documentation. So, it seems it may be visible on AIX, even if they have never taken the time to a is adding dd it to the getcwd() manual page.

Confusion #1 in adding this


.. versionadded:: 3.8


.. function:: lchflags(path, flags)

Set the flags of *path* to the numeric *flags*, like :func:`chflags`, but do
Expand Down
25 changes: 24 additions & 1 deletion Lib/os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
__all__ = ["altsep", "curdir", "pardir", "sep", "pathsep", "linesep",
"defpath", "name", "path", "devnull", "SEEK_SET", "SEEK_CUR",
"SEEK_END", "fsencode", "fsdecode", "get_exec_path", "fdopen",
"popen", "extsep"]
"popen", "extsep", "get_current_dir_name"]

def _exists(name):
return name in globals()
Expand Down Expand Up @@ -651,6 +651,29 @@ def get_exec_path(env=None):
return path_list.split(pathsep)


def get_current_dir_name():
"""
Return a string representing the current working directory taking into
consideration the users *PWD* environment variable if it exists. This
is opposed to getcwd() which dereferences symlinks in the path. This
function is identical to getcwd() on systems that do *not* support
the *PWD* environment variable.
"""
cwd = getcwd()

Copy link
Member

Choose a reason for hiding this comment

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

I think empty lines in this functions are redundant. Empty lines inside functions are used for separating groups of statements in large functions, but this function is enough small and simple. And will be smaller after removing empty lines.

try:
pwd = environ["PWD"]
except KeyError:
return cwd

cwd_stat, pwd_stat = map(stat, [cwd, pwd])
Copy link
Member

Choose a reason for hiding this comment

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

Would be clearer just to write two stat calls.


if (cwd_stat.st_dev == pwd_stat.st_dev and
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use samefile()?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, confusion #2 -, assuming that get_current_dir_name() is supported by the platform why is that not just being called rather than rewriting it? I admit my confusion comes from the lack of platform documentation, but why so hard? Wouldn't calling the "platform" implementation be more efficient than writing this in "pure" python.

More simply, why not use the platform implementation, perhaps adding it to posixmodule.c rather than here?

cwd_stat.st_ino == pwd_stat.st_ino):
return pwd
return cwd


# Change environ to automatically call putenv(), unsetenv if they exist.
from _collections_abc import MutableMapping

Expand Down
42 changes: 42 additions & 0 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,48 @@ def tearDownClass(cls):
os.rmdir(support.TESTFN)


class CurrentDirTests(unittest.TestCase):

def setUp(self):
self.pwd = os.environ['PWD']
base = os.path.abspath(support.TESTFN)
self.tmp_dir = base + '_dir'
self.tmp_lnk = base + '_lnk'

def test_getcwd(self):
# os.getcwd() always returns the dereferenced path
with support.temp_cwd(self.tmp_dir):
os.chdir(self.tmp_dir)
self.assertEqual(self.tmp_dir, os.getcwd())
Copy link
Member

Choose a reason for hiding this comment

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

Swap arguments. The first argument is an actual value, the second argument is an expected value.

os.symlink(self.tmp_dir, self.tmp_lnk, True)
os.chdir(self.tmp_lnk)
self.assertEqual(self.tmp_dir, os.getcwd())
os.environ['PWD'] = self.tmp_dir
self.assertEqual(self.tmp_dir, os.getcwd())
os.environ['PWD'] = self.tmp_lnk
self.assertEqual(self.tmp_dir, os.getcwd())
os.unlink(self.tmp_lnk)
Copy link
Member

Choose a reason for hiding this comment

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

The link will be leaked in the case of failure. It is better to use addCleanup(). Add the following line just before creating a link.

self.addCleanup(support.unlink, self.tmp_lnk)


def test_get_current_dir_name(self):
# os.get_current_dir_name() returns the direct path--mirroring
# the PWD environment variable if it exists regardless of
# whether the path contains symlinks.
with support.temp_cwd(self.tmp_dir):
os.environ['PWD'] = self.tmp_dir
self.assertEqual(self.tmp_dir, os.get_current_dir_name())
os.symlink(self.tmp_dir, self.tmp_lnk, True)
if os.name == 'posix':
os.environ['PWD'] = self.tmp_lnk
self.assertEqual(self.tmp_lnk, os.get_current_dir_name())
else:
os.environ['PWD'] = self.tmp_lnk
self.assertEqual(self.tmp_dir, os.get_current_dir_name())
os.unlink(self.tmp_lnk)

def tearDown(self):
os.environ['PWD'] = self.pwd


class RemoveDirsTests(unittest.TestCase):
def setUp(self):
os.makedirs(support.TESTFN)
Expand Down
0