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

feat: move directories while updating buffer paths (supercharged :move) #12923

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

satoqz
Copy link
Contributor

@satoqz satoqz commented Feb 20, 2025

:move is nice, but it doesn't let you move a directory and automatically update the buffer paths for any files inside of it that are opened in the editor. Currently this is rather cumbersome, you need to :sh mv foo bar, close all affected buffers, then reopen them again at their new paths.

This PR proposes a couple of changes:

  1. Editor::move_path now handles updating the paths for all affected buffers when it moves a directory.
  2. :move takes 1 to n arguments. If a single argument is given, the argument is the destination path and the buffer path is the source (this is the existing behavior, which continues to work as before). If multiple arguments are given, :move behaves like the Unix mv command: :move foo bar moves foo to bar, :move foo bar baz moves foo and bar into baz.
  3. :move foo bar knows to move foo to bar/foo if bar is an existing directory. Again this mirrors the behavior of Unix mv and should be more intuitive.

I think this is a natural extension to the existing behavior and has worked very well for me during testing so far. Any suggestions or feedback is appreciated :)

@satoqz
Copy link
Contributor Author

satoqz commented Feb 28, 2025

Now rebased on top of #12527.


@nik-rev I think my changes to Editor::move_path could be interesting for your #12902 PR. Instead of fs::rename you could call Editor::move_path (since it now supports both files and directories) and automatically get some LSP support.

A general note regarding LSP support: Complex LSP edits across multiple files (like, moving a directory and bulk updating import paths in a bunch of affected files) can only be supported if we also watch all of those files, register them with the LSP and then have a way to apply LSP edits to files that are not open in buffers (likely by automatically open them in a buffer with the edits applied?)

We can technically already support those kinds of LSP edits if 1) all the affected files are already opened as buffers and 2) we include the entire list of those files in the events sent in move_path, but I don't think that is a great solution, and LSP watched files support would be preferable.

@nik-rev
Copy link
Contributor

nik-rev commented Feb 28, 2025

Now rebased on top of #12527.

@nik-rev I think my changes to Editor::move_path could be interesting for your #12902 PR. Instead of fs::rename you could call Editor::move_path (since it now supports both files and directories) and automatically get some LSP support.

A general note regarding LSP support: Complex LSP edits across multiple files (like, moving a directory and bulk updating import paths in a bunch of affected files) can only be supported if we also watch all of those files, register them with the LSP and then have a way to apply LSP edits to files that are not open in buffers (likely by automatically open them in a buffer with the edits applied?)

We can technically already support those kinds of LSP edits if 1) all the affected files are already opened as buffers and 2) we include the entire list of those files in the events sent in move_path, but I don't think that is a great solution, and LSP watched files support would be preferable.

Thanks, Editor::move_path is what I'm using in that PR now. It is very simple to migrate, and nice to host the logic for rename / move of paths in a single place -- so my PR does not need to consider this

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.

2 participants