Added command to rename current document, with lsp support #5531

ontley · 2023-01-14T12:15:37Z

Includes willRenameFiles and didRenameFiles requests to the language server.
Honestly not sure if didRenameFiles is even needed here

Should file changes suggested by the language server be automatically saved if the file didn't have any other changes?

the-mikedavis · 2023-01-14T14:24:30Z

There is already #4514 but it doesn't cover the LSP renaming

cor · 2023-01-18T20:31:38Z

Nice one! Really happy that it includes LSP rename support 👍🏻

ontley · 2023-01-18T22:07:17Z

I added the rnm alias for the command, referring to #4514 (comment)

ontley · 2023-01-18T22:11:52Z

I wonder if it would be good to add a feature to automatically save suggested changes from the lsp if a file didn't have changes before, saves the trouble of opening every file and saving manually, or perhaps a status message saying which files changed?

…t changes on rename

devgioele · 2023-02-13T13:01:06Z

Looking forward to this!

edrex · 2023-03-23T00:55:51Z

I need to test this, but do suggested changes cause non-open files to be opened?

I wonder if it would be good to add a feature to automatically save suggested changes from the lsp if a file didn't have changes before, saves the trouble of opening every file and saving manually

IMO it's not needed, as :wa is easy enough, and there's already editor.auto-save. I get wanting transactionality, since the rename is immediate, but IDT it warrants a special case.

ontley · 2023-03-24T20:16:31Z

I need to test this, but do suggested changes cause non-open files to be opened?

I wonder if it would be good to add a feature to automatically save suggested changes from the lsp if a file didn't have changes before, saves the trouble of opening every file and saving manually

IMO it's not needed, as :wa is easy enough, and there's already editor.auto-save. I get wanting transactionality, since the rename is immediate, but IDT it warrants a special case.

Ah I didn't even know about :wa, I guess it's not needed then

edrex · 2023-03-26T04:45:23Z

Ah I didn't even know about :wa

It's a vimism. There's also :wqa.

ontley · 2023-03-27T19:16:36Z

Should the lsp errors be displayed in the status bar, along with the message that the file was successfully renamed?

ghost · 2023-07-07T07:30:56Z

Why it is not merged already?

ontley · 2023-07-07T07:37:22Z

Why it is not merged already?

Waiting for reviews

ghost · 2023-07-07T07:39:14Z

Why it is not merged already?

Waiting for reviews

Can anyone review or contributors only?

ontley · 2023-07-07T07:44:41Z

Why it is not merged already?

Waiting for reviews

Can anyone review or contributors only?

Only "reviewers with write access" and I can't even request reviews

pascalkuthe · 2023-07-07T12:46:51Z

helix-term/src/commands/typed.rs

@@ -2752,6 +2812,13 @@ pub const TYPABLE_COMMAND_LIST: &[TypableCommand] = &[
            fun: reset_diff_change,
            signature: CommandSignature::none(),
        },
+        TypableCommand {
+            name: "rename",
+            aliases: &["rnm"],


Suggested change

aliases: &["rnm"],

aliases: &["mv"],

I think rnm is a bit of a weird alias and not commonly used. Considering how widely used the shell command is I think mv could be a good alias

If we do set the alias to move, we're probably better of with calling the command itself move or move_file

we have other commands that have totally different aliases too so I think leaving it like that is fine too

pascalkuthe · 2023-07-07T12:47:17Z

helix-term/src/commands/typed.rs

+        TypableCommand {
+            name: "rename",
+            aliases: &["rnm"],
+            doc: "Rename the currently selected buffer",


Suggested change

doc: "Rename the currently selected buffer",

doc: "Move the current buffer and its corresponding file to a different path",

I think we need to highlight here that this is different than just :w new_path. Ofcourse this just a short help string but I think calling it move is generally clearer

helix-term/src/commands/typed.rs

pascalkuthe · 2023-07-07T12:56:34Z

helix-term/src/commands/typed.rs

+        .canonicalize()
+        .map_err(|_| anyhow!("Could not get absolute path of the document"))?;
+    let mut path_new = path.clone();
+    path_new.set_file_name(OsStr::new(new_name.as_ref()));


Suggested change

path_new.set_file_name(OsStr::new(new_name.as_ref()));

path_new.parent().expect("is a file").join(new_name);

I think we should allow moving to new paths not just new filenames

note that this means that the destination directory might not exist so we need rename! too

This is a sort of weird one, if we change the command name to move as previously said, probably a good ide, thoguh mv in bash takes absolute paths, not ones relative to the file but I think it's work

On the other hand I don't think a file renaming function should be able to move it as most file managers just let you rename it

Imo, move is probably better

I guess it might make mose sense to just call the entire command move and make the destination a path relative to CWD. If somebody wants to just change the filename they can use %filname/../<new_name> in the future.

I think that generally move is a better idea than rename because it can do more (and the LS can handle full moves for example RA can move a module to a totally different module

pascalkuthe · 2023-07-07T13:04:51Z

helix-term/src/commands/typed.rs

+                        if apply_workspace_edit(cx.editor, helix_lsp::OffsetEncoding::Utf8, &edit)
+                            .is_err()
+                        {
+                            log::error!(":rename command failed to apply edits")


we already show an error message herer you don't need to display it. The returned result is just forcases where we need to communicate to the LSP that the edit failed (not here)

helix-term/src/commands/typed.rs

pascalkuthe · 2023-07-07T13:08:46Z

helix-term/src/commands/typed.rs

+    doc.set_path(Some(path_new.as_path()))
+        .map_err(|_| anyhow!("File renamed, but could not set path of the document"))?;
+
+    if let Some(lsp_client) = doc.language_server() {


the wii rename request must be sent (and processed) before the rename happens so this needs to be moved to the start of the function:

The will rename files request is sent from the client to the server before files are actually renamed as long as the rename is triggered from within the client either by a user action or by applying a workspace edit. The request can return a WorkspaceEdit which will be applied to workspace before the files are renamed.

At the end of the function you must send didRename:

The did rename files notification is sent from the client to the server when files were renamed from within the client.

you also need to support mullti LSP here. Probably ok to just send wiiRename to the first server that supports it and didRename to all of them

pascalkuthe · 2023-07-07T13:12:10Z

helix-lsp/src/client.rs

+    }
+
+    pub fn did_rename_files(&self, params: &[lsp::FileRename]) -> impl Future<Output = Result<()>> {
+        let files = params.to_owned();


this must check the server capabiliy workspace.fileOperations.didRename

pascalkuthe · 2023-07-07T13:15:13Z

helix-lsp/src/client.rs

+        &self,
+        params: &Vec<lsp::FileRename>,
+    ) -> impl Future<Output = Result<lsp::WorkspaceEdit>> {
+        let files = params.to_owned();


you need to check workspace.fileOperations.willRename herer (or even better at the callsite to support multiple LSPs)

pascalkuthe · 2023-07-07T13:17:49Z

Why it is not merged already?

Waiting for reviews

Can anyone review or contributors only?

you are probably new to opensource. A PR needs to be reviewed by a miantainer before its merged. Only maintainers can review PRs. We can't just trust random people with code review. There are hundreds of issues and PRs for us to get through so it can take a while.

ghost · 2023-07-07T13:45:37Z

Why it is not merged already?

Waiting for reviews

Can anyone review or contributors only?

you are probably new to opensource. A PR needs to be reviewed by a miantainer before its merged. Only maintainers can review PRs. We can't just trust random people with code review. There are hundreds of issues and PRs for us to get through so it can take a while.

yeah, I am new. Thanks for providing information about reviews!

Co-authored-by: Pascal Kuthe <[email protected]>

wetheredge · 2023-08-02T23:46:43Z

helix-term/src/commands/typed.rs

+    let (_, doc) = current!(cx.editor);
+    let path = doc
+        .path()
+        .ok_or_else(|| anyhow!("Scratch buffers can not be renamed, use :write"))?


Suggested change

.ok_or_else(|| anyhow!("Scratch buffers can not be renamed, use :write"))?

.ok_or_else(|| anyhow!("Scratch buffers can not be renamed, use :write"))?;

ontley added 2 commits January 14, 2023 11:28

Added rename command

79a5d35

Added an error if the new path already exists

398a820

ontley marked this pull request as draft January 14, 2023 13:26

ontley added 3 commits January 14, 2023 14:44

Formatting

25ca3d5

Fixed wrong command name being used

3b23489

fixed clippy suggestions

fdef4f2

ontley marked this pull request as ready for review January 14, 2023 16:23

ontley changed the title ~~Added command to rename current document~~ Added command to rename current document, with lsp support Jan 14, 2023

removed didRenameFiles call, fixed early return due to path Err

258aaa5

cor mentioned this pull request Jan 18, 2023

Add rename command. #4514

Closed

added ':rnm' alias to ':rename'

6ef8fad

ontley added 3 commits January 22, 2023 11:11

code cleanup

406fd88

merged with master, fixed an incorrect error if the lsp didn't sugges…

289220c

…t changes on rename

formatting

9526d7c

edrex mentioned this pull request Feb 15, 2023

Feature request: rename files when using the rename action artempyanykh/marksman#153

Open

archseer mentioned this pull request Mar 9, 2023

File explorer and tree helper (v3) #5768

Open

archseer added this to the next milestone Mar 10, 2023

ontley added 2 commits March 24, 2023 20:28

removed debug line

826d9b7

merged with master

37d57b0

ontley closed this Mar 24, 2023

ontley reopened this Mar 24, 2023

ontley added 2 commits March 24, 2023 21:19

cargo fmt

17904c6

docgen

e1d628d

pascalkuthe modified the milestones: 23.03, next Mar 27, 2023

merged with master

403a38b

ontley marked this pull request as draft July 7, 2023 07:46

ontley marked this pull request as ready for review July 7, 2023 07:46

pascalkuthe requested changes Jul 7, 2023

View reviewed changes

pascalkuthe added A-language-server Area: Language server client S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. A-command Area: Commands C-enhancement Category: Improvements labels Jul 7, 2023

ontley and others added 6 commits July 7, 2023 21:19

Improved new buffer error message

9925f3e

Co-authored-by: Pascal Kuthe <[email protected]>

Removed unnecessary path normalizing

e787750

Co-authored-by: Pascal Kuthe <[email protected]>

Update helix-term/src/commands/typed.rs

411be74

Co-authored-by: Pascal Kuthe <[email protected]>

Update helix-term/src/commands/typed.rs

a9a30c1

Co-authored-by: Pascal Kuthe <[email protected]>

Update helix-term/src/commands/typed.rs

c820566

Co-authored-by: Pascal Kuthe <[email protected]>

Update helix-term/src/commands/typed.rs

c0e7f50

Co-authored-by: Pascal Kuthe <[email protected]>

wetheredge reviewed Aug 2, 2023

View reviewed changes

yo-main mentioned this pull request Oct 21, 2023

Add command to move files with LSP support #8584

Merged

ontley closed this Oct 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added command to rename current document, with lsp support #5531

Added command to rename current document, with lsp support #5531

ontley commented Jan 14, 2023 •

edited

Loading

the-mikedavis commented Jan 14, 2023

cor commented Jan 18, 2023

ontley commented Jan 18, 2023

ontley commented Jan 18, 2023

devgioele commented Feb 13, 2023

edrex commented Mar 23, 2023

ontley commented Mar 24, 2023

edrex commented Mar 26, 2023 •

edited

Loading

ontley commented Mar 27, 2023

ghost commented Jul 7, 2023

ontley commented Jul 7, 2023

ghost commented Jul 7, 2023

ontley commented Jul 7, 2023

pascalkuthe Jul 7, 2023

ontley Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

ontley Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe Jul 7, 2023

pascalkuthe commented Jul 7, 2023

ghost commented Jul 7, 2023

wetheredge Aug 2, 2023

	doc: "Rename the currently selected buffer",
	doc: "Move the current buffer and its corresponding file to a different path",

	path_new.set_file_name(OsStr::new(new_name.as_ref()));
	path_new.parent().expect("is a file").join(new_name);

	.ok_or_else(\|\| anyhow!("Scratch buffers can not be renamed, use :write"))?
	.ok_or_else(\|\| anyhow!("Scratch buffers can not be renamed, use :write"))?;

Added command to rename current document, with lsp support #5531

Added command to rename current document, with lsp support #5531

Conversation

ontley commented Jan 14, 2023 • edited Loading

the-mikedavis commented Jan 14, 2023

cor commented Jan 18, 2023

ontley commented Jan 18, 2023

ontley commented Jan 18, 2023

devgioele commented Feb 13, 2023

edrex commented Mar 23, 2023

ontley commented Mar 24, 2023

edrex commented Mar 26, 2023 • edited Loading

ontley commented Mar 27, 2023

ghost commented Jul 7, 2023

ontley commented Jul 7, 2023

ghost commented Jul 7, 2023

ontley commented Jul 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pascalkuthe commented Jul 7, 2023

ghost commented Jul 7, 2023

Choose a reason for hiding this comment

ontley commented Jan 14, 2023 •

edited

Loading

edrex commented Mar 26, 2023 •

edited

Loading