-
-
Notifications
You must be signed in to change notification settings - Fork 189
languages/csharp: add razor support #1271
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
base: main
Are you sure you want to change the base?
Conversation
|
Could you target the v0.8 branch? The LSP experience was mostly overhauled and improved, so this PR will likely benefit from the new API. |
|
I've noticed a lot has changed. I’m working on it! 😁 |
|
we merged 0.8 into main! So you can target main directly, now (sorry for the double switch) |
9e90201 to
c48c342
Compare
|
I figured it out! It wasn't a roslyn-ls version problem. Razor wasn't working correctly because the .razorExtension wasn't being mapped. Now everything works perfectly. |
c48c342 to
9887f11
Compare
9887f11 to
99f459f
Compare
|
I actually added roslyn-ls in v8.0 already, you should be able to use it now that's pulled into main. |
I noticed. It’s a pretty good implementation. I’ve just added Razor support. Your implementation doesn’t have that, and I need it for Blazor projects. |
horriblename
left a comment
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.
Thanks, but overall, I think we should wait for the nixpkgs PR to land first before adding razor support (see my other comments)
modules/plugins/languages/csharp.nix
Outdated
| (mkIf (cfg.lsp.enable && lib.elem "roslyn" cfg.lsp.servers) { | ||
| vim.luaConfigRC.roslyn-path = '' | ||
| -- NOTE: this is required by roslyn-nvim to find roslyn | ||
| vim.env.PATH = vim.env.PATH .. ":${pkgs.roslyn-ls}/bin" |
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.
- add the package to
vim.extraPackagesinstead
2. the package needs to be overridable somehow, preferably withdoesn't really apply if we wait for the nixpkgs PRvim.lsp.servers.roslyn.cmd, or if it's really not doable, add an optionvim.languages.csharp.lsp.package(this deviates from our usual pattern so an appropriate warning in casecmdis used would be appreciated)
modules/plugins/languages/csharp.nix
Outdated
| vim.env.MASON = vim.fn.expand("~/.local/share/nvf/mason") | ||
| local expanded_mason = vim.fn.expand("$MASON") | ||
| local mason = expanded_mason == "$MASON" | ||
| and vim.fs.joinpath(vim.fn.stdpath("data"), "mason") | ||
| or expanded_mason | ||
| local mason_packages = vim.fs.joinpath(mason, "packages") | ||
| -- Define target paths | ||
| local targets = { | ||
| vim.fs.joinpath(mason_packages, "roslyn", "libexec", ".razorExtension"), | ||
| vim.fs.joinpath(mason_packages, "roslyn-unstable", "libexec", ".razorExtension"), | ||
| } |
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.
use vim.fn.stdpath("data") .. "/mason" instead of hard-coding ~/.local/share
I don't really like this approach overall. I would rather wait until https://github.com/NixOS/nixpkgs/issues/472357 is merged. Will probably need to patch out the warning in roslyn-nvim unless they start supporting it themselves though
| omnisharp = ["omnisharp-extended-lsp-nvim"]; | ||
| csharp_ls = ["csharpls-extended-lsp-nvim"]; | ||
| roslyn_ls = []; | ||
| roslyn = ["roslyn-nvim"]; |
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.
this is technically a breaking change, a warning about removal of "roslyn_ls" in lsp.servers would be nice
034f317 to
7f9bb33
Compare
It adds roslyn-ls configuration and roslyn-nvim plugin
7f9bb33 to
4cad097
Compare
I’ve rethought the symlink approach, and I now believe this solution is more reproducible and, overall, more stable. |
It adds the roslyn-ls configuration and the roslyn-nvim plugin. Razor support works partially. The roslyn-nvim team announced that they now support Razor, but I think the version of roslyn-ls available in nixpkgs isn’t up to date. Because of that, Razor files may still produce errors. Aside from this, everything works fine.
This PR should fix #1268
Sanity Checking
nix fmt).#nix(default package).#maximal.#docs-html(manual, must build).#docs-linkcheck(optional, please build if adding links)x86_64-linuxaarch64-linuxx86_64-darwinaarch64-darwin