-
Notifications
You must be signed in to change notification settings - Fork 88
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
LSP formatting without calling the topiary binary #1526
Conversation
lsp/nls/README.md
Outdated
(this is covered in the setup instructions). | ||
[topiary](https://github.com/tweag/topiary). NLS exposes a formatting capability | ||
when compiled with the `format` Cargo feature, which is enabled by default. No | ||
configuration or external dependencies are necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
lsp/nls/src/requests/mod.rs
Outdated
pub mod goto; | ||
pub mod hover; | ||
pub mod symbols; | ||
|
||
#[cfg(feature = "format")] | ||
pub mod formatting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a feature flag? I thought those were mainly for keeping the core cheaply embeddable, but it's fine if nls is big...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jneem has a point. While you could argue that more features just means more configurability, this does has cost in sprinkling #[cfg(feature = "format")]
(and #[cfg(..)]
is a quite rigid mechanism). I'm fine either way but it's a good question to ask if NLS without formatting is something that could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a great argument for the feature flag. Let's just do without and see if someone complains; adding it back in shouldn't be too hard if that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Testing this is also surprisingly straightforward. In general, at least until now, I feel like we have hit a sweet spot in our testing infrastructure: quite simple (TOML-based meta-comments) but still extensible and powerful enough.
lsp/nls/src/requests/mod.rs
Outdated
pub mod goto; | ||
pub mod hover; | ||
pub mod symbols; | ||
|
||
#[cfg(feature = "format")] | ||
pub mod formatting; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jneem has a point. While you could argue that more features just means more configurability, this does has cost in sprinkling #[cfg(feature = "format")]
(and #[cfg(..)]
is a quite rigid mechanism). I'm fine either way but it's a good question to ask if NLS without formatting is something that could be useful.
Co-authored-by: Yann Hamdaoui <[email protected]>
With this change formatting in the LSP uses topiary as a library instead of calling an external binary, just like the implementation of
nickel format
. The result is formatting capability in the LSP without any external dependencies enabled by default.