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 'fileToRename' property to RenameInfo #24702

Merged
4 commits merged into from
Sep 10, 2018
Merged

Add 'fileToRename' property to RenameInfo #24702

4 commits merged into from
Sep 10, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jun 5, 2018

Fixes #24501

Adds a fileToRename property to RenameInfo so that the editor can rename the file instead.

@ghost ghost requested review from DanielRosenwasser and mjbvz June 5, 2018 19:41
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I'll get initial support for this in VSCode this month so people using typescript@next can start testing

* fileName to rename.
* If set, `getEditsForFileRename` should be called instead of `findRenameLocations`.
*/
fileToRename?: string;
Copy link
Contributor

@mhegazy mhegazy Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a folder as well.. e.g.

// modules/a/index.ts

export var a;
import * from "./modules/a";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should handle this case too.

And since we are renaming a module, we should also rename its references in tsconfig.json like "files"/"include"/"paths" etc..

I think we should just call in getEditsForFileRename instead of computing something on the fly.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Sep 5, 2018

@Andy-MS please address conflicts + Mohamed's comments

@ghost
Copy link
Author

ghost commented Sep 10, 2018

@mjbvz Good to merge?

@mjbvz
Copy link
Contributor

mjbvz commented Sep 10, 2018

Yes, we don't have support in VS Code for this yet but I'll add it this iteration

@ghost ghost merged commit 24a5bdd into master Sep 10, 2018
@ghost ghost deleted the fileToRename branch September 10, 2018 18:25
@mjbvz
Copy link
Contributor

mjbvz commented Sep 10, 2018

@Andy-MS So is the expected behavior that when triggering rename on an import path in a file, we actually trigger the rename in the vscode file explorer?

@ghost
Copy link
Author

ghost commented Sep 10, 2018

You could do that, which I'm guessing is the simplest way. Another option is to do the rename inline as usual, and change the name of the file without opening the file explorer.

@mjbvz
Copy link
Contributor

mjbvz commented Sep 11, 2018

Ok, just so I make sure I understand what is supported:

  1. Can you only rename the file itself or can you trigger a rename on directory in the path, such as renaming dir in import {} from './dir/file'

  2. As part of the rename, should I ever be able to change the path, e.g. doing a rename from import {} from './file' to import {} from './sub/renamedFile'

@ghost
Copy link
Author

ghost commented Sep 12, 2018

  1. We trigger the rename on the entire string span, not on some part enclosed by /. (That could be changed if you want.) But if the import specifier is "./foo" and it imports from "./foo/index.ts", we'll try to rename the directory.
  2. I think so.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants