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-35951: os.renames() creates directories if original name doesn't exist #11827

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@nanjekyejoannah
Copy link
Contributor

nanjekyejoannah commented Feb 12, 2019

I have added a fix to resolve os.rename() creating directories when original directory doesn't exist.

https://bugs.python.org/issue35951

try:
os.rename(path1, path1new)
self.assertFalse(path.exists(path1new))
except FileNotFoundError:

This comment has been minimized.

@matrixise

matrixise Feb 12, 2019

Contributor

not sure about this line, but you can use the contextlib.suppress function, like that

with contextlib.suppress(FileNotFoundError):
    os.rename(path1, path1new)
    self.assertFalse(path.exists(path1new))
@@ -263,8 +263,9 @@ def renames(old, new):
"""
head, tail = path.split(new)
if head and tail and not path.exists(head):
makedirs(head)
if path.exists(old):

This comment has been minimized.

@matrixise

matrixise Feb 12, 2019

Contributor

why not?

if path.exists(old) and head and tail and not path.exists(head):
    makedirs(head)
@vstinner
Copy link
Member

vstinner left a comment

IMHO the doc becomes outdated by your change, and should be rephrased:

I also suggest to add a ".. versionchanged:: x.y (...)" to os.renames() doc.

Since the current surprising behavior is documented, I suggest to only make the change in Python 3.8 and not backport it. So the doc note would be ".. versionchanged:: 3.8 (...)".

I would also prefer to document the change at https://docs.python.org/dev/whatsnew/3.8.html#changes-in-the-python-api since it's "somehow" backward incompatible (feature or bugfix? hard to say here :-)).

@@ -0,0 +1 @@
`os.rename()` now doesnt create directories when original directory doesn't exist.

This comment has been minimized.

@vstinner

vstinner Feb 12, 2019

Member
Suggested change Beta
`os.rename()` now doesnt create directories when original directory doesn't exist.
`os.rename()` no longer create destination subdirectories when the original directory doesn't exist.
@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Feb 12, 2019

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.

path1 = 'temp/not-exists'
path1new = 'temp/test2/test3/test4'
try:
os.rename(path1, path1new)

This comment has been minimized.

@vstinner

vstinner Feb 12, 2019

Member

I don't understand your test. Do you expect FileNotFoundError? If yes, you have to use:

with self.assertRaises(FileNotFoundError):
    os.rename(path1, path1new)
self.assertFalse(path.exists(path1new))

Currently, "self.assertFalse(path.exists(path1new))" is never executed if I understand correctly!

This comment has been minimized.

@giampaolo

giampaolo Feb 12, 2019

Contributor

Actually you are testing os.rename, not os.renames. Also (unrelated to this change per se):

  1. it appears there are no tests for os.renames at all
  2. also os.rename test isn't actually testing the actual functionality

I will file a ticket for that but I would recommend having them before proceeding with this.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Feb 12, 2019

@pablogsal, @giampaolo: This change is somehow backward incompatible, but IMHO the current behavior is wrong and the new behavior is correct. What do you think?

@pablogsal pablogsal self-requested a review Feb 12, 2019

@@ -126,11 +126,21 @@ def test_closerange(self):
@support.cpython_only
def test_rename(self):
path = support.TESTFN
not_exists = 'temp2/test'

This comment has been minimized.

@giampaolo

giampaolo Feb 12, 2019

Contributor

this seems unnecessary

path1 = 'temp/not-exists'
path1new = 'temp/test2/test3/test4'
try:
os.rename(path1, path1new)

This comment has been minimized.

@giampaolo

giampaolo Feb 12, 2019

Contributor

Actually you are testing os.rename, not os.renames. Also (unrelated to this change per se):

  1. it appears there are no tests for os.renames at all
  2. also os.rename test isn't actually testing the actual functionality

I will file a ticket for that but I would recommend having them before proceeding with this.

@giampaolo

This comment has been minimized.

Copy link
Contributor

giampaolo commented Feb 12, 2019

@vstinner I replied on BPO.

Unrelated, while I'm here. I'm taking a look at test_os.py and I would like to add more tests for fs-related os functions (it appears they're kinda lacking). Also feel free to CC me in filesystem related issues (e.g. os, shutil modules). Thanks.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Feb 13, 2019

@giampaolo thanks for insight on tests. I dint catch that at all :)

I am working on tests for os.renames() I opened an issue to track that here : https://bugs.python.org/issue35982.

IMHO, I think we need to improve the current behavior.

The discussion on the BPO is not conclusive. You pointed out that this fix makes the problem a lot less likely to occur but we may end up with a race condition.

Can we close this PR so that maybe someone comes with a fix that addresses both problems? can we work with this fix? Am happy to comply with any decision. cc @vstinner

Once we agree on this, I will make necessary changes also in the review of this PR.

@nanjekyejoannah

This comment has been minimized.

Copy link
Contributor Author

nanjekyejoannah commented Feb 18, 2019

Am closing this till there is consensus or someone else can propose a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment