elixir-lsp / vscode-elixir-ls

Elixir language support and debugger for VS Code, powered by ElixirLS.
https://marketplace.visualstudio.com/items?itemName=JakeBecker.elixir-ls
MIT License
545 stars 106 forks source link

feat: improve terminal file link handling #419

Closed 03juan closed 5 months ago

03juan commented 6 months ago

Hi and thanks for a great library.

I ran into a couple of instances where the extension wasn't generating terminal links or opening them correctly. Here is my attempt at fixing them within the current implementation.

The first video is the current functionality and the second shows all 3 fixed links.

https://github.com/elixir-lsp/vscode-elixir-ls/assets/4416345/5e01c08b-fed2-42e0-8e13-6ca788f15f84

https://github.com/elixir-lsp/vscode-elixir-ls/assets/4416345/41c0478a-c352-4ca1-9bb6-ef1a9e4fe655

03juan commented 6 months ago

This won't work with path: dependencies or core libs like :elixir or :iex, that would require caching the output of Mix.Project.deps_paths/0 and Application.app_dir/1 for the various core apps from the LSP's side. Is this something you'd be insterested in?

lukaszsamson commented 6 months ago

Nice one

This won't work with path: dependencies or core libs like :elixir or :iex, that would require caching the output of Mix.Project.deps_paths/0 and Application.app_dir/1 for the various core apps from the LSP's side. Is this something you'd be insterested in?

Possibly. We can add a custom LSP command in the language server that returns what is needed. Keep in mind that some people do not have stdlib files on their filesystems

03juan commented 6 months ago

Keep in mind that some people do not have stdlib files on their filesystems

The fs.stat check on L55 will take care of this. The function will return null on file not found and fail the if.

03juan commented 5 months ago

@lukaszsamson thanks for the merge but did you check out the link in the reply to your comment? it has a more generalised regex that also improves Version catching. I made it to show the improvements with a few localised tests, since I couldn't get the repo's test tasks working to add them there

lukaszsamson commented 5 months ago

Yes, we can address that in a follow-up PR