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 a code action to create nonexistent linked files #239

Merged
merged 3 commits into from
Jul 25, 2023

Conversation

jparoz
Copy link
Contributor

@jparoz jparoz commented Jul 9, 2023

Here's my first pass at a new code action to create files which are linked, but don't exist in the workspace (i.e. a goto-def on the link would error). Choosing the code action creates the file in the root of the workspace folder.

Closes #219.

A few notes:

  • I modified LanguageServerProtocol/Types.fs to support the create file operation (and other file resource changes). I added a new type for each type of resource operation, and added them all to a new [<ErasedUnion>] sum type DocumentChange. This type then replaced TextDocumentEdit where appropriate. I'm not 100% sure that this is the best way to go about this, but it works and is fairly neat.
  • A particular wart in the above is that each of these new types has the field Kind: string; I would rather use the existing type ResourceOperationKind, but I couldn't figure out how to get those to be serialised into JSON in lowercase (i.e. they would become "Create" instead of "create"). Any pointers on how to make this happen using ResourceOperationKind would be awesome; in particular, I tried using JSON.NET's JsonPropertyAttribute and NamingStrategy with no change in the output.
  • createNonexistentLink in CodeActions.fs is the function which carries out the main logic for this action. I've used the Start of the range parameter given to the code action as the position which checks for a link; I'm not sure if this should use the start, or the end of the range, or something else? I'm also happy to take any style feedback, especially as I'm pretty fresh to F#.
  • To support this I added a function ensureMarkdownExt to Misc.fs which ensures that a path has a configured Markdown extension, or adds the first configured extension if there is none. This is similar to chopMarkdownExt.
  • I also added this code action to the handler in Server.fs; keen for style advice here as well.
  • The rest of the changes are adding this code action as a configurable option, which is pretty boilerplatey. The only thought I had was whether it would be useful to allow configuring where the created file is located (e.g. in a subdirectory)? Currently all files created with this action are just dumped in the workspace folder, which works for my use case, but might not for others.

The major problem that I still have with this implementation is that once the file is created, Marksman doesn't notice that fact straight away. In particular, if I try to goto-def on the link to the just-created file, it still gives an error. If I manually open the file (using Neovim's e command in my case), then the link works with goto-def. I think the workspace state isn't being updated when we do this code action; I would have thought that this might be triggered by the client when it creates the file upon receiving the CreateFile request, but it seems not. It's possible that this could be a bug in Neovim, that it's not sending DidCreateFiles notifications, but I don't have any other client configured at the moment to be able to test for that. If you have any knowledge of what might be the path forward, that would be super helpful.

@artempyanykh
Copy link
Owner

Thanks for the PR @jparoz! I'll have a closer look shortly. Meantime, could you look into CI failures -- there are some type issues in tests.

@jparoz
Copy link
Contributor Author

jparoz commented Jul 20, 2023

This now passes on my machine. Sorry I didn't check this earlier!

Copy link
Owner

@artempyanykh artempyanykh left a comment

Choose a reason for hiding this comment

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

A few minor comments, but otherwise great work! (Also do run make fmt && make check)

Some thoughts about the problem you mentioned where after the file had been created, the server still didn't see it. I did a bit of testing and assuming that VSCode is a reference implementation of an LSP client:

  1. In VSCode after the code action runs and the client creates a file, it sends a notification to the server that the file was created: workspace/didCreateFiles.
  2. In Emacs with lsp-mode there's a similar behavior, although the notification is different: instead of file creation it says textDocument/didOpen.
  3. Emacs with eglot, nvim, and helix all fail to send a notification about the new file.

Overall, since the LSP server sets file watches for .md files, I feel that VSCode's behavior is correct and the rest of the clients are off.

I did a quick read of the spec and couldn't find a way to "ask the client to tell the server about a file that the server knows the client just created" (sigh). I'd say the best would be to merge this as-is and open issues in respective editor's repos linking to this convo.

LanguageServerProtocol/Types.fs Show resolved Hide resolved
Marksman/CodeActions.fs Outdated Show resolved Hide resolved
Marksman/CodeActions.fs Outdated Show resolved Hide resolved
Marksman/CodeActions.fs Outdated Show resolved Hide resolved
Marksman/Server.fs Outdated Show resolved Hide resolved
Marksman/Config.fs Outdated Show resolved Hide resolved
Marksman/Server.fs Outdated Show resolved Hide resolved
@jparoz
Copy link
Contributor Author

jparoz commented Jul 24, 2023

Thanks for this feedback! I’ll incorporate the changes (and run make check and make fmt 🤦🏼‍♂️). Thanks for helping me through learning F#! 😁

Run `make fmt` and `make check`
Rename CreateNonexistentLink to CreateMissingFile throughout
Add a helper module to create instances of `DocumentChange`
Use different `kind`s for different code actions
@jparoz
Copy link
Contributor Author

jparoz commented Jul 25, 2023

I think I've taken all the feedback into account in the last commit, and hopefully the workflow should pass this time. 😅

@artempyanykh
Copy link
Owner

Looking great, thanks @jparoz! I'm going to cut a new release shortly.

Would you like to create an issue in neovim regarding the missing file notifications?

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.

Code action to create nonexistent files which are linked
2 participants