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(als): Show GPS project file in LspInfo #1693

Closed
wants to merge 1 commit into from

Conversation

TamaMcGlinn
Copy link
Contributor

Add extra info to the als LspInfo to show which GPR project is being used. When there is none or more than one, LspInfo shows this. In that case, als will not work.

gpr_display

@TamaMcGlinn TamaMcGlinn force-pushed the als_show_gpr_project branch 2 times, most recently from 2dcfcb1 to 8328dea Compare February 6, 2022 10:32
@TamaMcGlinn
Copy link
Contributor Author

Any issues merging this?

@TamaMcGlinn TamaMcGlinn force-pushed the als_show_gpr_project branch 2 times, most recently from 2723d02 to a3ccf5e Compare February 15, 2022 16:20
@TamaMcGlinn
Copy link
Contributor Author

Can you merge this please? It makes als much more useful in neovim.

@mjlbach
Copy link
Contributor

mjlbach commented Mar 1, 2022

I'm currently in the process of trying to remove server specific things, and make lspconfig a generic, vim-lsp-settings-like minimal repository. I'm interested in pushing people to create server specific plugins that can provide more tailored experience. I also don't have much time for neovim atm, but I really do not want to encourage LspInfo becoming something beyond a basic debugging tool for servers.

@TamaMcGlinn
Copy link
Contributor Author

so, would you rather I create a separate nvim-als-lspconfig plugin, that depends on nvim-lspconfig, or includes its sources? Also, what's the motivation for this? It is obviously much easier to just click 'merge' than to think about how to split off each lsp configuration server, but I guess you have some strong motivations for wanting that.

@mjlbach
Copy link
Contributor

mjlbach commented Mar 1, 2022

so, would you rather I create a separate nvim-als-lspconfig plugin, that depends on nvim-lspconfig, or includes its sources?

I'd rather an ada.nvim that provided the same functionality that lspconfig does, namely addressing your other concerns about workspaceFolders/project system modules and providing any project specific info in its own UI.

Also, what's the motivation for this?

Namely, I want to keep the scope of lspconfig limited. There are many features users request, but would be unsustainable to have in a catch-all plugin like lspconfig (and poorly implemented). Things like:

  • python virtual environment selection
  • support for rust-analyzer's inlay hints
  • showing the active GPS project file
  • handling system libraries as workspace folders for various language servers/platforms.

The goal is to move any useful infrastructure from lspconfig to core, and then you can build a tailored language plugin that leverages the neovim lsp API.

It's only 300 lines of lua code you would need to copy out of lspconfig to replicate it's functionality for your plugin, which is fine so long as it's Apache-2.0 license and the source is attributed.

It is obviously much easier to just click 'merge'

If just clicking merge was my approach to being a maintainer, I think I'd be a rather poor one.

@justinmk justinmk closed this May 11, 2022
@TamaMcGlinn
Copy link
Contributor Author

TamaMcGlinn commented May 12, 2022

@justinmk I would still rather this be merged.

I want to keep the scope of lspconfig limited

While this argument sounds reasonable, my fixes were both in lua/lspconfig/server_configurations/als.lua and did not affect any other plugins. There are plenty of examples of such features being accepted and merged for languages that are in use by the plugin's maintainers. Yet, the argument could stand equally well for disallowing any fix or update that is only to a specific language config. So you have to recognize there is a double standard here.

I understand I could create a separate als-config plugin that copies a bunch of code from nvim-lspconfig and works properly. However, there is no advantage to leaving als.lua here without the feature. It is easier and better to just maintain a fork of nvim-lspconfig with Ada support added; like I said, the only things I change are in the ada.lua file, so I still don't see any problem in merging.

On the one hand, I understand not wanting to spend any time on language support for a language you don't personally use, but if that's the consideration; why keep als.lua at all? Or any other language, for that matter? I would be happy to be the maintainer for the specific lsp config of the language I am using, but it's not very nice to tell me you want to keep the scope of your plugin limited, and thereby reject any updates to the als.lua config, especially when it is broken like this.

@TamaMcGlinn
Copy link
Contributor Author

also note that I have been using this particular closed PR, rebased onto the latest master, for almost half a year now with no modifications necessary or issues encountered. If you do merge, and someone opens an issue; of course I will jump on that and fix it. I'm not asking you to maintain als.lua - just to accept fixes / updates like you do for ccls, eslint and others.

@TamaMcGlinn
Copy link
Contributor Author

Just checking in again, as I just rebased my fork onto master again; I still feel this belongs here. Any ALS user would want this feature built-in.

Please clarify how ALS is so different from all the other language server configs, in that you want its configuration in a separate repository, instead of welcoming PRs here that improve the nvim-lspconfig supplied config. What you are proposing would leave it up to users to add yet another plugin with duplicated code from nvim-lspconfig, with no apparent benefit.

Also see issue 1683, which was erroneously closed. The repro steps, I'm sure, are encountered by every new user of ALS because the issue uproots even the most basic usage of ALS. I mention this because this PR, while it doesn't fix the issue, at least it shows the user what the issue is, by showing an error message in LspInfo, stating there are 0 or multiple gpr files.

@TamaMcGlinn
Copy link
Contributor Author

@justinmk Any issues merging this currently?

@TamaMcGlinn
Copy link
Contributor Author

If this PR needs any changes before merging, please give me specific feedback on the code. Today I had another look and trimmed out a few lines of unnecessary code, but it doesn't seem that this PR was closed for any specific reason, so I'm not sure what needs doing in order to integrate this into the master branch.

I hope everyone can see that 'keeping the scope of lspconfig limited' is not achieved by rejecting all contributed updates to the als configuration. If you wanted to split out lspconfig-core and have separate plugins for each languageserver, that would be fine, but that doesn't seem to be what you are doing. As long as you are accepting changes to any languageserver config, you should also accept changes to the ALS config. The other part of this argument, put forward by @mjlbach, was to put my proposed change in a list among very different changes, where each of the others are for specific setups using the corresponding languageserver, which not all users of that languageserver would need - in contrast, als always needs a GPR file, and searches for it automatically in the same manner as this PR's code does, so every user of ALS would find my PR useful diagnostics, especially since having the wrong gpr file, or ambiguity, is fatal for ALS. (see issue #1683 - this PR does not fix it, but at least makes it a lot easier to debug. for a fix, see nvim-lsp-gpr-selector)

Please, if you are using ALS, check if all of this works for your purposes. If not, you should have no issue merging this - I have been using this daily for six months already, so I'm pretty confident it works.

@justinmk
Copy link
Member

justinmk commented Jul 19, 2022

See #1937 (comment) . We are not currently equipped to position nvim-lspconfig as a vscode replacement (btw: vscode itself requires installing purpose-built extensions for each language, it doesn't provide all of these random customizations). The purpose of nvim-lspconfig was to help users get started, so that Nvim LSP basic features work. Not to provide a full custom experience for each kind of LSP server.

Some custom commands made their way into nvim-lspconfig, but that doesn't change the above. Maintaining customizations for hundreds of configs is not sustainable and not in the purview of nvim-lspconfig.

Furthermore, customizing :LspInfo is out of scope regardless of where we land with #1937 .

glepnir pushed a commit that referenced this pull request Aug 22, 2024
To move towards the goal of having lsp configs decentralised
and lower the maintenance burden on current maintainers, as
requested by @glepnir, @mjlbach and @justinmk

See #1693
and #1683
and #3275 (comment)
glepnir pushed a commit that referenced this pull request Sep 20, 2024
Moved ALS config into own repo. See issue #1683
regarding problems with the current config.
lspconfig is not intended to provide functional
lsp configs, but rather a framework for other
plugins to implement those.
For more about that, see the discussion on
#1693

The new ALS LSP config plugin is
https://github.com/TamaMcGlinn/nvim-lspconfig-ada
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.

3 participants