Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

Revive rls.toml #1047

Closed
mickaelistria opened this issue Sep 7, 2018 · 8 comments
Closed

Revive rls.toml #1047

mickaelistria opened this issue Sep 7, 2018 · 8 comments

Comments

@mickaelistria
Copy link

I was notified that rls.toml was not supported any more and that instead RLS-specific messages should be sent over the LSP. I think it's a mistake for the following reasons (copied from rust-lang/vscode-rust#107 (comment)

Relying on specific elements in didChangeConfiguration require every clients to develop a specific integration for RLS (to present and send the right options), while relying on a file allows customization without specific development, and even allowed users to deal with the configuration by themselves with any tool.
The approach of relying on client to customize stuff is inferior technically and in term of UX in several cases to the approach of editing a file.
Trust me, I'm working on Eclipse IDE, and we got a lot of requests like "I just want .editorconfig", "I just want to edit the pom file"...). End-users like to deal with configuration files over tool-specific stuff, it makes the project more portable to other tools and users, and specific nodes/configurations in a Language Server can quickly turn the LS into a client-specific LS as the other ones may not be able to catch up.
Also, in term of maintainability, let's imagine you add a feature in RLS that's configurable, then to configure it, you'll basically want all clients to make specific development. That doesn't scale with the number of clients and for example, unless it becomes business critical to anyone, Eclipse Corrosion won't be able to develop and maintain preference pages and specific messages as you create them, and in worst case, we'd have to stop the project because the RLS is not really standardized on free-to-adopt processes and protocols because of its own layer of configuration.
Allowing configuration via a file allows to adopt new configuration easily: if users want it differently, they can tweak the file, no specific development involved. Specific tools can create richer editors or views for the file if they want, but it's not a requirement.
Overall, by relying too much on the configuration approach of the protocol, RLS is introducing a lot of RLS-specific concepts instead of relying on the standard part the protocol that can be expected to be supported by any client. If you feel like the protocol is missing some support for some standard operations, please try to make them specified operations in the protocol instead of over-using the didChangeConfiguration node.
The current pattern is IMO worrying for the ability of the majority of tools to implement Rust support without too much pain. In the end, only the ref implementation of RLS client (VSCode) will be remaining. I'm not sure it's really the goal of RLS to only support VSCode properly and to not be configurable on Eclipse IDE, Eclipse Che, Theia and so on.
I suggest you revive support for .rls.toml files in order to make RLS usable beyond VSCode-specific integration.

@norru
Copy link

norru commented Oct 6, 2018

@norru
Copy link

norru commented Oct 6, 2018

@mickaelistria would it be possible to add a "simple" configuration option in Corrosion that just reads an arbitrary file (given its path) and sends its payload to the RLS as a didChangeConfiguration message? By default this could be something like ~/.rls/corrosion-rls.json

@mickaelistria
Copy link
Author

This pragmatically sounds like a good idea. And not something too hard to implement.
I just think at the moment, it could require new API in LSP4E.
But I still would like to to remind that this would be an IDE-specific integration and that instead, having this in RLS would be better and more profitable design.

@norru
Copy link

norru commented Oct 8, 2018

@mickaelistria I do agree, it's probably easier to follow the path of least resistance as it is obvious that the RLS team are not keen on having integration points other than the host LSP application. Must be The Microsoft Way™ ;)

@norru
Copy link

norru commented Dec 1, 2018

@norru
Copy link

norru commented Dec 3, 2018

Fixed by eclipse-corrosion/corrosion#183 (kinda)

@norru
Copy link

norru commented Dec 5, 2018

@mickaelistria shall we close this? We have our own rls.conf now.

@mickaelistria
Copy link
Author

@norru: I think so. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants