Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
I'm leaning against landing this right now. It's so much code and it seems like it would be super intrusive as you're typing. That may just be a preference though. I'm still considering it but I need some convincing. |
I’ll configure it locally for my own shell and use it for a while on a daily basis to check if it is worth it and if it can be made less untrusive. |
wvhulle
force-pushed
the
inline-hints
branch
2 times, most recently
from
39fbb6f to
f5c7dec
Compare
|
I made the diagnostics a bit less intrusive by keeping labels shorter, dimming colors, see this image: 
|
|
@fdncred You can test on this branch (with recent Nu-lint binary): cargo run --example lsp_diagnostics --features lsp_diagnostics |
|
Fix menu with alt+f: 
|
|
@wvhulle Would you like to add the code action feature to |
Isn’t nu-lsp an lsp server and this a small lsp client? Nu-lsp could be used as the real-time diagnostics provider (as lsp server) for this client instead of nu-lint. So you wouldn’t be bound to nu-lint with the current implementation and the actual lsp server is swappable. If I would add code actions to nu-lsp (although I don’t mind doing that) it would not be possible to have this type of real-time diagnostics and fixes in the reedline prompt. It’s a slightly different feature I think? |
That's what I meant, better to reuse what's already present.
Why's that? I haven't checked your current implementation, but AFAIK, whether it's realtime or not depends simply on when you send the lsp request in the REPL loop. |
|
Showing realtime diagnostics while typing a prompt in reedline requires more than just adding code actions to nu-lsp. They seem to be orthogonal projects. |
I don't get it, what's the special source on the nu-lint side to make it real-time? |
|
@wvhulle I think you misunderstood my point. I'm not saying we don't need this lsp-client on reedline side. |
|
There is no special source. Nu-lint is an extension of nu-lsp to some extent. However nu-lint has more lint rules and fixes that could assist new users of nu shell and indirectly reedline prompt. This draft pr does not require nu-lint but could also be used with nu-lsp. Maybe you can try the example I added? |
examples/lsp_diagnostics.rs
Outdated
| //! Run with: cargo run --example lsp_diagnostics --features lsp_diagnostics | ||
| //! | ||
| //! Prerequisites: | ||
| //! - nu-lint must be installed and available in PATH with LSP support |
There was a problem hiding this comment.
You mean nu-lint is not necessary and could be replaced with nu-lsp?
There was a problem hiding this comment.
This particular example references nu-lint binary but it is easy to switch out and replace with nu-lsp.
There was a problem hiding this comment.
Fair enough, I think I've made myself clear. Please don't make this feature defaults to using a non-official lsp server.
There was a problem hiding this comment.
I understand, however, the official LSP does not have many auto-fixes available (as far as I know), so nu-lint is kind of needed in the example to show how the fix menu feature works (with any LSP that has fixes).
There was a problem hiding this comment.
Feel free to enhance nu-lsp then.
I mean it's OK to leave this example depending on nu-lint. But don't make this whole feature that way.
There was a problem hiding this comment.
Feel free to enhance
nu-lspthen.
I'd love to, but some people use nu-lint in CLI mode (locally or in CI), which seems to be out of scope for nu-lsp.
There was a problem hiding this comment.
Feel free to enhance
nu-lspthen.I'd love to, but some people use nu-lint in CLI mode (locally or in CI), which seems to be out of scope for nu-lsp.
As you said, it's swappable, right? People who favor nu-lint can config in their own ways. And you can continue working on it, but I don't see the beauty of it. I'd prefer an all-in-one and configurable nu-lsp and all people contributing to it instead of a divided community.
There was a problem hiding this comment.
Whether nu-lsp or nu-lint are the default, or whether nu-lsp is the official LSP server seems a bit unrelated to this PR to me. No? This PR is just about adding a minimal LSP client in Reedline, which is unrelated to LSP servers. The screenshots however, should give an idea how it might look to the users when the LSP client is active.
There was a problem hiding this comment.
Whether nu-lsp or nu-lint are the default, or whether nu-lsp is the official LSP server seems a bit unrelated to this PR to me. No? This PR is just about adding a minimal LSP client in Reedline, which is unrelated to LSP servers. The screenshots however, should give an idea how it might look to the users when the LSP client is active.
If it's configurable, then yes, it's unrelated.
But as I pointed out, this comment said nu-lint is marked as prerequisite, that's why we're having this conversation.
Fix rendering in narrow terminals of diagnostics Add multi-char spans
The prompt width calculation was including ANSI escape sequences (color codes like \x1b[32m) in the width, inflating the offset and causing diagnostic underlines to appear too far to the right. Added strip_ansi() helper to remove ANSI sequences before measuring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update the lsp_diagnostics example to use the same REEDLINE_LS environment variable that nu-cli uses, making it consistent with the actual nushell setup. Also improved error messages when the env var is not set.
Export craneLib in lib outputs so nushell can optionally use the same crane configuration for consistent builds.
OpenDiagnosticFixMenu now returns Inapplicable instead of Handled when there are no code actions at the cursor position. This allows UntilFound keybindings to fall through to the next event (e.g., completion menu). Previously, Tab would open an empty diagnostic menu and block normal completions even when no diagnostics existed.
wvhulle
force-pushed
the
inline-hints
branch
3 times, most recently
from
7b49421 to
2e333d1
Compare
|
Actually I think this feature is more useful in editors than in the REPL. I don't care the code quality of those typed interactively, and I find the realtime changing ui distracting. Besides, the requests are blocking, right? I'd prefer a more responsive shell. But that's just my personal preference, meaning that if you PR to |
I'll make it non-blocking. |
wvhulle
force-pushed
the
inline-hints
branch
2 times, most recently
from
a33d72e to
56292ac
Compare
Share the rendering of the asynchronous diagnostics with the external_printer feature in the top-level event loop
That's cool, but would you like to wait for some review feedbacks before you move on? |
Of course, I can wait a bit. |
|
Is there a way to test it without nu-lint? I tried and get this: Error: REEDLINE_LS environment variable not set.
Set it to the full LSP server command (e.g., "nu-lint --lsp").
Example: REEDLINE_LS="nu-lint --lsp" cargo run --example lsp_diagnostics --features lsp_diagnostics |
Thanks, I just want to make sure we all agree on the direction to go. And maybe making the PR a bit easier to review. |
|
I created this GIF for fun so people could see what it looks like as you're typing. @wvhulle I really do think this is just so cool and amazing work. Nice job! The question we have to figure out is do we want it in reedline or nushell as a configurable option or feature. I've asked the core-team for input and hope to get back to you soon. Thanks for thinking about reedline/nushell and trying to make it so much better! ❤️ |
|
We talked about this quite a bit yesterday in our meeting. Everyone agrees that this is a really cool example of what you can do with reedline. However, we just think this is too much of a distraction for users. For that reason, we're closing this PR. Thanks for your great work! Closing this PR makes me wonder if reedline should have some type of plugin architecture to allow others to experiment and use cool tools like nu-lint without all that code being embedded into reedline. 🤔 |
|
@fdncred are you sure it’s not just nu-lint that is too noisy? Because nu-lint is indeed very noisy with 150 pedantic lints enabled by default. Maybe a much smaller subset of enabled lints might be useful? |
|
@wvhulle It wasn't solely up to me. The core-team decided against it. The idea is that linting is great in an editor but having it on the repl gets in the way and may be too confusing for people. This is why I was suggesting that maybe a way forward would be to implement some type of plugin architecture to reedline so that people could inject plugins at various time to do a variety of things. I'm not sure that's a perfect solution either but it's all I could really think of. |
|
Okay then. I'll keep working on it on my fork though. Do we move the discussion somewhere else for a plugin system? |
|
ya, it may be good to make a new issue @wvhulle. Thanks. |
3 participants
This PR adds a small LSP client that can be constructed from a path to any LSP server binary.
The screenshots in this thread are demonstrations on how user interaction with this LSP client would look.
I am looking for feedback on how to show the diagnostics and auto-fixes. Do you have any suggestions?
Fix menu:
There is an example provided that uses the
nu-lintLSP server because it has a lot of lints and fixes available and this shows how multiple violations are rendered vertically and horizontally. It is possible to usenu-lspbutnu-lsphas fewer rules, so the density of the user interface cannot be tested so much.