GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1154351: Add get_current_dir_name() to os module by bradengroom · Pull Request #10117 · python/cpython

Conversation

@bradengroom

@bradengroom bradengroom commented Oct 26, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Co-authored-by: Marc Adam Anderson [email protected] .

https://bugs.python.org/issue1154351

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bradengroom
bpo-1154351: add get_current_dir_name() to os module
1daae93 
Co-authored-by: Marc Adam Anderson <[email protected]> .
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 26, 2018
@bedevere-bot bedevere-bot added the awaiting review label Oct 26, 2018
@bradengroom bradengroom changed the title Add get_current_dir_name() to os module bpo-1154351: Add get_current_dir_name() to os module Oct 26, 2018
bradengroom added 3 commits October 25, 2018 19:46
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
ee4c4f6
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
f836691
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
89e1984
serhiy-storchaka
serhiy-storchaka reviewed Oct 26, 2018
View reviewed changes
Comment thread Doc/library/os.rst Outdated
function is identical to :func:`getcwd()` on systems that do **not**
support the ``PWD`` environment variable.

.. availability:: Unix.

@serhiy-storchaka serhiy-storchaka Oct 26, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@aixtools aixtools Oct 26, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/os.py Outdated
except KeyError:
return cwd

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

@serhiy-storchaka serhiy-storchaka Oct 26, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/os.py Outdated

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

if (cwd_stat.st_dev == pwd_stat.st_dev and

@serhiy-storchaka serhiy-storchaka Oct 26, 2018

Copy link
Copy Markdown
Member

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()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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())

@serhiy-storchaka serhiy-storchaka Oct 26, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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)

@serhiy-storchaka serhiy-storchaka Oct 26, 2018

Copy link
Copy Markdown
Member

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
aixtools
aixtools reviewed Oct 26, 2018
View reviewed changes
Comment thread Lib/os.py Outdated

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

if (cwd_stat.st_dev == pwd_stat.st_dev and

@aixtools aixtools Oct 26, 2018

Copy link
Copy Markdown
Contributor

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bradengroom added 4 commits October 26, 2018 21:31
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
4bfb33c
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
963659a
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
cc1735f
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
d49fdf3
bradengroom
bradengroom commented Oct 27, 2018
View reviewed changes
Comment thread Lib/test/test_os.py
os.symlink(self.tmp_dir, self.tmp_lnk, True)
os.chdir(self.tmp_lnk)
if os.name == 'nt':
# windows doesn't dereference the path

@bradengroom bradengroom Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bradengroom
bradengroom commented Oct 27, 2018
View reviewed changes
Comment thread Lib/os.py
cwd = getcwd()

if name == 'nt':
return cwd

@bradengroom bradengroom Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bradengroom

bradengroom commented Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka I've addressed your feedback and the tests are passing now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Oct 27, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@serhiy-storchaka serhiy-storchaka Oct 27, 2018

Copy link
Copy Markdown
Member

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:?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@serhiy-storchaka serhiy-storchaka Oct 27, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/os.py Outdated
the *PWD* environment variable.
"""
cwd = getcwd()

@serhiy-storchaka serhiy-storchaka Oct 27, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/os.py
"""
cwd = getcwd()

if name == 'nt':

@serhiy-storchaka serhiy-storchaka Oct 27, 2018

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradengroom bradengroom Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asottile asottile Oct 28, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bradengroom bradengroom Oct 28, 2018

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@asottile asottile Oct 28, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2018-10-25-19-44-42.bpo-1154351.KhXLos.rst
@@ -0,0 +1 @@
Add get_current_dir_name() to the os module.

@serhiy-storchaka serhiy-storchaka Oct 27, 2018

Copy link
Copy Markdown
Member

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 ..."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bradengroom
fixup! bpo-1154351: add get_current_dir_name() to os module
17386e1
@bradengroom

bradengroom commented Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka Thanks for the quick feedback. I addressed your comments here:
17386e1
#10117 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Oct 28, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vstinner
vstinner requested changes Oct 28, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Oct 28, 2018

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting review labels Oct 28, 2018
@morell5

morell5 commented Sep 9, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

@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"?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner

vstinner commented Sep 9, 2019

Copy link
Copy Markdown
Member

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.
morell5 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner closed this Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@vstinner vstinner vstinner requested changes
+2 more reviewers
@asottile asottile asottile left review comments
@aixtools aixtools aixtools left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@bradengroom @bedevere-bot @morell5 @vstinner @asottile @serhiy-storchaka @aixtools @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.