-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
fix rename cause import change wrongly #38462
Changes from 2 commits
c048371
d0927a8
a806801
3b370c4
61ce269
53d5894
3eb8b5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -595,7 +595,10 @@ namespace vfs { | |
if (existingNode) { | ||
if (isDirectory(node)) { | ||
if (!isDirectory(existingNode)) throw createIOError("ENOTDIR"); | ||
if (this._getLinks(existingNode).size > 0) throw createIOError("ENOTEMPTY"); | ||
// if both old and new arguments point to the same directory, just pass. So we could rename /src/a/1 to /src/A/1 in Win. | ||
// if not and the directory pointed by the new path is not empty, throw an error. | ||
// However, in the link in the description, it says: If the old argument points to the pathname of a file that is not a directory, the new argument shall not point to the pathname of a directory. If the link named by the new argument exists, it shall be removed and old renamed to new. In this case, a link named new shall remain visible to other threads throughout the renaming operation and refer either to the file referred to by new or old before the operation began. Write access permission is required for both the directory containing old and the directory containing new. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is too long to read easily in a PR. Please split this comment across multiple lines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove it and the content is as follows. In the link http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html |
||
if (this.stringComparer(oldpath,newpath)!== 0 && this._getLinks(existingNode).size > 0) throw createIOError("ENOTEMPTY"); | ||
ShuiRuTian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
else { | ||
if (isDirectory(existingNode)) throw createIOError("EISDIR"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/// <reference path='fourslash.ts' /> | ||
|
||
// @Filename: /a/b/file1.ts | ||
////import { foo } from "foo"; | ||
|
||
// @Filename: /a/node_modules/foo/index.d.ts | ||
////export const foo = 0; | ||
|
||
verify.getEditsForFileRename({ | ||
oldPath: "/a/b", | ||
newPath: "/a/B", | ||
newFileContents: {}, | ||
}); |
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.
Wait this seems incorrect.
sourceDirectory
is already canonical path.https://github.com/microsoft/TypeScript/pull/38462/files#diff-aa86ac6315ba3e1d25a9a1789f7f5a78L110
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.
Since this is the only substantive change outside of tests in this PR, is there something we are doing incorrectly when computing
sourceDirectory
? If not, what does this PR change other than the VFS in the test harness and adding a test?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 we would need to diagnose why source directory is not canonical already instead.
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.
#38462 (comment)
Would it be better to add a comment to let users know it is their responsibility to make sure sourceDirectory canonical path?