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 rename command. #4514

Closed
wants to merge 15 commits into from
Closed

Conversation

grv07
Copy link
Contributor

@grv07 grv07 commented Oct 29, 2022

#4393

Add rename command :rename new_name.txt

helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
helix-term/src/commands/typed.rs Outdated Show resolved Hide resolved
"Root path must be the same."
);

ensure!(!new_path.exists(), "File already exists.");
Copy link
Member

Choose a reason for hiding this comment

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

There could be a :rename!/:rnm! version that skips this check so that renaming can overwrite existing files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we can add that too.

I will continue and add them in a separate PR.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@the-mikedavis Pls let me know if you have any questions on this.

the-mikedavis
the-mikedavis previously approved these changes Oct 30, 2022
@the-mikedavis the-mikedavis added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 30, 2022
@kirawi
Copy link
Member

kirawi commented Nov 29, 2022

I think we could also use the LSP call:
#4393 (comment)

@cor
Copy link
Contributor

cor commented Jan 18, 2023

I think we should merge this PR instead, as it contains proper LSP support: #5531 , although it would be nice to add the shorthand :rnm from this PR to it

@the-mikedavis
Copy link
Member

Merged in #8584 which covers this and the language server bits

@ontley
Copy link
Contributor

ontley commented Nov 10, 2023

I think that's an "issue" with the language servers themselves, not the helix lsp client.

I say "issue" because you're using an lsp feature without waiting for the language server to load properly

@David-Else
Copy link
Contributor

I think that's an "issue" with the language servers themselves, not the helix lsp client.

I say "issue" because you're using an lsp feature without waiting for the language server to load properly

It still happens after the language server has loaded, note:

If you wait for ltex-ls to fully load before moving then the new file then you get 'no code actions available' but the text seems to have inherited the old underline errors?

@ontley
Copy link
Contributor

ontley commented Nov 10, 2023

That I'm not 100% certain on as rust-analyzer handles everything correctly, I think the language servers aren't responding nicely to didRenameFile, could you test a different language with lsp support?

It still happens after the language server has loaded, note:

Do you still get the errors in the log?

@David-Else
Copy link
Contributor

@ontley I commented in the wrong thread, moved it to #8584 . I am away so can't continue this conversation now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants