-
Notifications
You must be signed in to change notification settings - Fork 35
feat: add command :TypescriptRenameFolder #59
base: main
Are you sure you want to change the base?
Conversation
a3b6a47
to
df41f30
Compare
df41f30
to
35a4a67
Compare
Thanks! At a glance this approach looks alright - I will test it out and give a more thorough review this weekend. |
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.
Apologies for the delay in reviewing this. The overall logic looks okay but there's one key issue with the current implementation as well as several other areas for improvement.
(opts) => { | ||
const sourceFile = vim.api.nvim_buf_get_name(bufnr); | ||
vim.ui.input( | ||
{ prompt: "Old path: ", default: sourceFile }, |
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 should default to the source file's parent directory and not the source file itself.
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.
what do you thing of using vim.ui.select
and prompting the user to select any of the parent directory to be renamed.
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.
Not sure about this - it would work well with Telescope / other pickers but might be kind of janky with the default handler, especially if there's a lot of choices. Let's keep it consistent with :TypescriptRenameFile
for now and consider that as a future improvement.
{ prompt: "Old path: ", default: sourceFile }, | ||
(sourceInput) => { | ||
if ( | ||
sourceInput === "" || |
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.
We can validate sourceInput
and targetInput
after prompting for both, since we should validate that they are not the same.
opts: Opts = {} | ||
): boolean => { | ||
debugLog(source, target); | ||
const sourceBufnr = vim.fn.bufadd(source); |
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 know if these 2 lines are still needed, since we would be loading a folder as a buffer.
|
||
if ( | ||
util.path.exists(target) && | ||
util.path.is_dir(target) && |
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 this check may cause issues, since we are not handling the case where the target exists but is not a directory (which should just throw an error).
debugLog(file); | ||
if ( | ||
file.endsWith(".ts") && | ||
renameFile(source + file, target + file, opts) |
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 doesn't seem to currently work since the combined paths don't include a separator. We can use nvim-lspconfig's utils.path.join
, or we can use vim.fs.find()
:
// files is an array of absolute paths
const files = vim.fs.find(
(file) => string.find(file, "[.][tj][s]x?$")[0] !== undefined,
{
path: source,
type: "file",
}
);
Then for the new path, we can replace source
with target
in the current path:
for (const file of files) {
debugLog(file);
if (renameFile(file, file.replace(source, target), opts)) {
debugLog("OK");
}
}
Though I think the above would look clearer if we change variable names as I mentioned in another comment.
} | ||
|
||
export const renameFolder = ( | ||
source: string, |
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'd prefer to rename these variables to something like sourceDir
and targetDir
to avoid confusion.
I really need this feature :) |
No description provided.