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

Set Up Config in LSP Mode #12454

Merged
merged 1 commit into from
Apr 14, 2024
Merged

Conversation

schrieveslaach
Copy link
Contributor

Description

When starting the LSP server, the configuration file and environment file are used to configure the LSP engine unless --no-config-file is provided.

This PR provides an improvement that is related to #10794

CC: @fdncred

@schrieveslaach schrieveslaach marked this pull request as draft April 8, 2024 20:17
@schrieveslaach schrieveslaach marked this pull request as ready for review April 8, 2024 20:17
@schrieveslaach
Copy link
Contributor Author

@fdncred, I was wondering if you wait a little with merging because I reviewed the changes here and I was wondering if we should implement workspace/configuration too with this PR. A client could provide an option to load or not to load the configuration for config folder.

What do you think?

@fdncred
Copy link
Collaborator

fdncred commented Apr 8, 2024

I'd be good with landing this one and then doing the workspace/configuration in a separate one. I haven't tried this one out but looking at the code, it seems like it may do what I'm talking about. Then we can tweak on that in a separate PR. How's that sound?

@schrieveslaach
Copy link
Contributor Author

Sounds great.

@fdncred
Copy link
Collaborator

fdncred commented Apr 9, 2024

I still get the same diagnostic errors after this change but I'm not sure why. Trying to think of another way to test this. 🤔

@fdncred
Copy link
Collaborator

fdncred commented Apr 9, 2024

If I do nu -c '$env.NU_LIB_DIRS' --lsp I get 'variable not found' errors pointing to $nu, which seems weird to me. Maybe in lsp mode, there isn't a fully startup of nushell with all the bells and whistles?

I get the same errors if I just do nu --lsp in a terminal. 🤔

I confirmed that before this PR, those errors do not show up. So something is not quite right here.

@schrieveslaach
Copy link
Contributor Author

@fdncred, I tried your commands on my Linux machine and they work just fine. If I remember correctly, you are on Windows, right? Could you try the version after rebase?

Maybe, I refactor the code so that it runs after initializing the LSP.

@fdncred
Copy link
Collaborator

fdncred commented Apr 13, 2024

@schrieveslaach I get this on MacOS. I can try Windows on Monday.
image

image

It looks like it's trying to work but it's not finding everything right or merging it? 🤔

When starting the LSP server, the configuration file and environment
file are used to configure the LSP engine unless --no-config-file is
provided.
@schrieveslaach
Copy link
Contributor Author

Ah, I think the LSP start was in the wrong spot. And I have no usage of $nu in env.nu. Can you give it another try?

@fdncred
Copy link
Collaborator

fdncred commented Apr 13, 2024

Seems to be working better because I don't have any red squigglies now. I'm not sure how to see what the lsp has configured for $env.NU_LIB_DIRS but I guess it must be reading my config/env files.
image

Before, these use lines would have diagnostic errors.

@fdncred fdncred marked this pull request as ready for review April 13, 2024 21:45
@fdncred
Copy link
Collaborator

fdncred commented Apr 13, 2024

Let me know if there's anything else you want to add, otherwise I'll land this.

@schrieveslaach
Copy link
Contributor Author

I will create another PR that will add workspaca/configuration.

@fdncred
Copy link
Collaborator

fdncred commented Apr 14, 2024

ok, thanks.

@fdncred fdncred merged commit 50b2dac into nushell:main Apr 14, 2024
15 checks passed
@fdncred fdncred added the LSP Language Server Protocol (nu-lsp) label Apr 14, 2024
@hustcer hustcer added this to the v0.93.0 milestone Apr 14, 2024
@schrieveslaach schrieveslaach deleted the lsp-config-files branch April 20, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP Language Server Protocol (nu-lsp)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants