Skip to content
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

Add shim for rename #1158

Merged
merged 3 commits into from Feb 14, 2020
Merged

Add shim for rename #1158

merged 3 commits into from Feb 14, 2020

Conversation

@divergentdave
Copy link
Contributor

divergentdave commented Jan 27, 2020

This adds a straightforward shim for rename, which is used by std::fs::rename. Testing is included.

As a heads up, I expect one or two merge conflicts between my PRs, since some of them touch the same use statements, or add items near the same places. I'll rebase and fix them as they come up.

@christianpoveda

This comment has been minimized.

Copy link
Contributor

christianpoveda commented Jan 29, 2020

this one looks good to me. I had some doubts about having null pointers but that case is being handled correctly.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2020

☔️ The latest upstream changes (presumably #1151) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 31, 2020

Please rebase instead of merging; we prefer not to have merge commits inside PRs.

@divergentdave divergentdave force-pushed the divergentdave:shim-rename branch from 094b711 to a3642eb Jan 31, 2020
@divergentdave

This comment has been minimized.

Copy link
Contributor Author

divergentdave commented Jan 31, 2020

OK, done

drop(file);
rename(&path1, &path2).unwrap();
assert!(path2.metadata().unwrap().is_file());
remove_file(&path2).ok();

This comment has been minimized.

Copy link
@RalfJung

RalfJung Feb 5, 2020

Member

Maye also test that path1 is gone?

Other than that, LGTM.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 8, 2020

☔️ The latest upstream changes (presumably #1159) made this pull request unmergeable. Please resolve the merge conflicts.

@divergentdave divergentdave force-pushed the divergentdave:shim-rename branch from a3642eb to f7e0857 Feb 9, 2020
@divergentdave

This comment has been minimized.

Copy link
Contributor Author

divergentdave commented Feb 14, 2020

I added the test for path1 and rebased

tests/run-pass/fs.rs Outdated Show resolved Hide resolved
Co-Authored-By: Ralf Jung <post@ralfj.de>
@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 14, 2020

Thanks! @bors r+

What would be nice (but doesn't block this PR) is to also test some cases where the function errors.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2020

📌 Commit 9186812 has been approved by RalfJung

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2020

⌛️ Testing commit 9186812 with merge 0a803c9...

bors added a commit that referenced this pull request Feb 14, 2020
Add shim for rename

This adds a straightforward shim for rename, which is used by `std::fs::rename`. Testing is included.

As a heads up, I expect one or two merge conflicts between my PRs, since some of them touch the same `use` statements, or add items near the same places. I'll rebase and fix them as they come up.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 0a803c9 to master...

@bors bors merged commit 9186812 into rust-lang:master Feb 14, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@divergentdave divergentdave deleted the divergentdave:shim-rename branch Feb 18, 2020
bors added a commit that referenced this pull request Feb 21, 2020
Test error case of std::fs::rename

As suggested [here](#1158 (comment)) this PR adds an additional test case for calling rename on a file path that doesn't exist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.