Skip to content

Conversation

@mehalter
Copy link
Contributor

The exposed autocommands make it very easy to add support for things such as the file operations defined in the LSP specification. Currently there are some events missing immediately before an operation is taken to support the rest of the events (workspace/willCreateFiles/workspace/willDeleteFiles/workspace/willRenameFiles). I'm sure there are other use cases as well where the user may want to hook into events right before an action is taken place.

@mehalter mehalter changed the title feat(files): add autocommand events immediately before performing file actions feat(files): add autocommand events before performing file actions Feb 19, 2025
@echasnovski
Copy link
Member

Thanks for the PR!

I plan to add support for LSP file create/rename/delete directly into 'mini.files', as it seems to be possible to do so relatively concisely. If that would be the case, I'd like to not expose another set of events. My main concern is that there is no guarantee that a file system operation will actually happen.

I'm sure there are other use cases as well where the user may want to hook into events right before an action is taken place.

Are you aware of any other such use cases? If not, I think I'd close this PR in favor of a future work I'd like to do in 'mini.files'.

@mehalter
Copy link
Contributor Author

I'm not sure off the top of my head of any other use cases of these events so all good if you want to just close this.

I'm also not sure how the specification guarantees validity in a rigorous manner for operations being unsuccessful. Especially because based on the semantics of the spec, the workspace edits also must be applied before the action takes place. So if you get the edit and apply it, I'm not sure of the means of reverting said edit in the case of the operation being unsuccessful (at least with the current neovim lua API).

Very exciting to hear you are adding the LSP support directly! I do agree it can be implemented relatively concise. One thing I would note with the implementation is to make sure to support the regex filtering on the files when deciding if you are sending the LSP requests to the client. I have seen several implementations of the LSP file operations and that is typically missed. If you are curious about some other implementations, AstroNvim's LSP plugin recently got an implementation in place that is pretty complete (and hopefully correct as well) that also implements some caching of the filtering resolution for performance: https://github.com/AstroNvim/astrolsp/blob/main/lua/astrolsp/file_operations.lua

@echasnovski
Copy link
Member

I'm also not sure how the specification guarantees validity in a rigorous manner for operations being unsuccessful. Especially because based on the semantics of the spec, the workspace edits also must be applied before the action takes place. So if you get the edit and apply it, I'm not sure of the means of reverting said edit in the case of the operation being unsuccessful (at least with the current neovim lua API).

Yes, that was one of my main concerns in the first place about adding Pre events and built-in support for willXxxxFiles support. Apart from somehow manually computing whether a file action will be successful (which I'd assume will be done mostly on a case-by-case basis) I am not aware of any automated and robust way of doing so.

My current starting idea is to issue a "counter request" in case an operation has failed:

  • willCreateFiles -> fail -> didCreateFiles + didDeleteFiles
  • wilRenameFiles -> fail -> didRenameFiles + didRenameFiles (with reversed names)
  • willDeleteFiles -> fail -> didDeleteFiles + didCreateFiles

Or, as didXxxFiles are notifications, maybe "counter request" with a single opposite willXxxFiles request.
Or, make "counter request" a full willXxxFiles + didXxxFiles. Not sure, needs experimentation.

Very exciting to hear you are adding the LSP support directly! I do agree it can be implemented relatively concise. One thing I would note with the implementation is to make sure to support the regex filtering on the files when deciding if you are sending the LSP requests to the client. I have seen several implementations of the LSP file operations and that is typically missed. If you are curious about some other implementations, AstroNvim's LSP plugin recently got an implementation in place that is pretty complete (and hopefully correct as well) that also implements some caching of the filtering resolution for performance: https://github.com/AstroNvim/astrolsp/blob/main/lua/astrolsp/file_operations.lua

Thanks, that's really helpful! I didn't know about custom filters... That'll be not that fun to implement, test, and maintain :(

Also, do you happen to know which LSP server has good {Will,Did}{Create,Rename,Delete}Files implementation? As in, they actually do something useful in the returned workspace edits and/or notifications. Preferably with built-in filters. That'll greatly simplify implementation and testing.


As said above, I am indeed inclined to close this in favor of (very hopeful that near) future built-in support for LSP file methods.

@mehalter
Copy link
Contributor Author

lua_ls uses didRename with filters provided, granted the filter is just the root directory <root_dir>/** with ignoreCase = true

vtsls impelemnts didRename with filters

rust-analyzer implements willRename with filters

basedpyright implements willRename with filters

there are a lot of language servers it seems that support didRename/willRename but I have searched and can't find any that actually use the {will,did}{Create,Delete}Files

I also think all of the instances I have checked that implement willRename or didRename only implement one of them rather than both of them

@echasnovski
Copy link
Member

Thanks, that's again very helpful!

It also does look similar to my brief search around a year-ish ago. As in: there are some servers that implement "rename" functionality, but none of "create"/"delete". So seeing how they would behave for failed file system actions is problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants