Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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
| exceptKeyError: | ||
| returncwd | ||
| 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_devand |
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 | ||
| withsupport.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()) | ||
| withmock.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 useaddCleanup(). 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_devand |
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) | ||
| ifos.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() | ||
| ifname=='nt': | ||
| returncwd |
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 thePWD environment variable on windows. I believe this matches the behavior documented.
bradengroom commentedOct 27, 2018
@serhiy-storchaka I've addressed your feedback and the tests are passing now. |
serhiy-storchaka left a comment
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() | ||
| ifname=='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:
getcwdon systems that donot support the :envvar:PWDenvironment variable.
Please correct me if I'm wrong since I'm less familiar with Windows, but I don't believe it uses thePWD environment variable in any way. I added that check so that we would not incorrectly change behavior fromgetcwd() if the user does set thePWD 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.
ingit bash on windows:
Anthony@AnthonysDesktop MINGW64 ~$ python -c 'import os; print(os.getenv("PWD"))'C:/Users/AnthonyThere 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 whetherpython is executed in the context of an interactive shell that sets or doesn't setPWD.
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 ..."
bradengroom commentedOct 27, 2018
@serhiy-storchaka Thanks for the quick feedback. I addressed your comments here: |
vstinner left a comment
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".
vstinner left a comment
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 closehttps://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 realisticuse cases. Not just "it would be nice to have this function".
bedevere-bot commentedOct 28, 2018
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 |
morell5 commentedSep 9, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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"? |
vstinner commentedSep 9, 2019
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. |
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Marc Adam Andersonmarc@marcadam.com .
https://bugs.python.org/issue1154351