-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
bpo-1154351: Add get_current_dir_name() to os module #10117
Conversation
Co-authored-by: Marc Adam Anderson <marc@marcadam.com> .
Doc/library/os.rst
Outdated
function is identical to :func:`getcwd()` on systems that do **not** | ||
support the ``PWD`` environment variable. | ||
|
||
.. availability:: Unix. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Lib/os.py
Outdated
except KeyError: | ||
return cwd | ||
|
||
cwd_stat, pwd_stat = map(stat, [cwd, pwd]) |
There was a problem hiding this comment.
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.
Lib/os.py
Outdated
|
||
cwd_stat, pwd_stat = map(stat, [cwd, pwd]) | ||
|
||
if (cwd_stat.st_dev == pwd_stat.st_dev and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use samefile()?
Lib/test/test_os.py
Outdated
# 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()) |
There was a problem hiding this comment.
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.
Lib/test/test_os.py
Outdated
self.assertEqual(self.tmp_dir, os.getcwd()) | ||
with mock.patch.dict('os.environ', {'PWD': self.tmp_lnk}): | ||
self.assertEqual(self.tmp_dir, os.getcwd()) | ||
os.unlink(self.tmp_lnk) |
There was a problem hiding this comment.
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)
Lib/os.py
Outdated
|
||
cwd_stat, pwd_stat = map(stat, [cwd, pwd]) | ||
|
||
if (cwd_stat.st_dev == pwd_stat.st_dev and |
There was a problem hiding this comment.
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?
os.symlink(self.tmp_dir, self.tmp_lnk, True) | ||
os.chdir(self.tmp_lnk) | ||
if os.name == 'nt': | ||
# windows doesn't dereference the path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a bug or the expected behavior. I haven't used windows in quite some time, but this is the behavior I'm seeing in CI.
cwd = getcwd() | ||
|
||
if name == 'nt': | ||
return cwd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this so that we don't look at the PWD
environment variable on windows. I believe this matches the behavior documented.
@serhiy-storchaka I've addressed your feedback and the tests are passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add also an entry in What's New.
Doc/library/os.rst
Outdated
.. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use :envvar:
?
Doc/library/os.rst
Outdated
|
||
Return a string representing the current working directory taking into | ||
consideration the users ``PWD`` environment variable if it exists. This is | ||
opposed to :func:`getcwd()` which dereferences symlinks in the path. This |
There was a problem hiding this comment.
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.
Lib/os.py
Outdated
the *PWD* environment variable. | ||
""" | ||
cwd = getcwd() | ||
|
There was a problem hiding this comment.
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.
""" | ||
cwd = getcwd() | ||
|
||
if name == 'nt': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you were performing review as I was writing a comment :)
#10117 (comment)
This function is documented with:
This function is identical to :func:
getcwd
on systems that do not support the :envvar:PWD
environment variable.
Please correct me if I'm wrong since I'm less familiar with Windows, but I don't believe it uses the PWD
environment variable in any way. I added that check so that we would not incorrectly change behavior from getcwd()
if the user does set the PWD
on Windows for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in git bash
on windows:
Anthony@AnthonysDesktop MINGW64 ~
$ python -c 'import os; print(os.getenv("PWD"))'
C:/Users/Anthony
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asottile
Thanks. Looks like my memory was incorrect. I'll fix this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure this function is useful/a good idea -- it's entirely dependent on whether python
is executed in the context of an interactive shell that sets or doesn't set PWD
.
I've commented on the bpo issue indicating that as well.
@@ -0,0 +1 @@ | |||
Add get_current_dir_name() to the os module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Patch by ..."
@serhiy-storchaka Thanks for the quick feedback. I addressed your comments here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now I have no opinion on the feature itself, but if we decide to add it, it should be added to the shutil module instead. os is a thin wrapper to C function, whereas shutil are functions based on the os module but adding "Python logic".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the purpose of this function. I suggest to reject this PR and close https://bugs.python.org/issue1154351
If someone needs this function, it can easily be reimplemented and copy/paste from the issue or this PR.
If someone really wants this feature to be added to Python stdlib, we need realistic use cases. Not just "it would be nice to have this function".
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@vstinner , may you make a key decision about this PR: close or provide requirements for merging the PR? The development of the task has stopped because there are no instructions from the core developer. As for me, I don't understand further actions on this task. Also I don't understand what "requested changes" bedeavere-bot was talking about? Where I can find these "requested changes"? |
The os module is thin wrappers to functions of the libc or even system calls. We don't implement heuristic using 2 variants of the same feature: there shutil module is there for that. Compare os.get_terminal_size() to shutil.get_terminal_size() for example. I close the PR to add os.get_current_dir_name(): IMHO it's more a feature for the shutil module. |
Co-authored-by: Marc Adam Anderson marc@marcadam.com .
https://bugs.python.org/issue1154351