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.

Sign up for GitHub

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

Jump to bottom

languages/nix: add nixd to supported LSP servers #458

Open
wants to merge 3 commits into
base: v0.7
Choose a base branch
from
Open

languages/nix: add nixd to supported LSP servers #458

wants to merge 3 commits into from

Conversation

Elias-Ainsworth
Copy link

@Elias-Ainsworth Elias-Ainsworth commented Nov 11, 2024 •
edited
Loading

Add nixd as a possible lsp for nix.

Sanity Checking

  • I have updated the changelog as per my changes.
  • I have tested, and self-reviewed my code.
  • Style and consistency
    • I ran Alejandra to format my code (nix fmt).
    • My code conforms to the editorconfig configuration of the project.
    • My changes are consistent with the rest of the codebase.
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas
    • I have added a section in the manual.
    • (For breaking changes) I have included a migration guide.
  • Package(s) built:
    • .#nix (default package)
    • .#maximal
    • .#docs-html
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

@Elias-Ainsworth
Add nixd.
3d1e38c 
Add nixd to flake.nix and legacyPackages.nix.

Format with alejandra.
@NotAShelf NotAShelf changed the title Add nixd. languages/nix: add nixd to supported LSP servers Nov 11, 2024
NotAShelf
NotAShelf requested changes Nov 11, 2024
View reviewed changes
Copy link
Owner

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM.

Left a few comments for consistency, if you could also add an entry to the changelogs this should be good to go.

flake.nix Outdated Show resolved Hide resolved
flake.nix Show resolved Hide resolved
modules/plugins/languages/nix.nix Show resolved Hide resolved
@NotAShelf
Copy link
Owner

Almost forgot, please rebase on the v0.7 branch as main is currently supposed to be in a feature freeze.

@diniamo
Copy link
Collaborator

diniamo commented Nov 11, 2024

There is a reaoson we didn't already have nixd, so we should be a little more cautious about merging this. I think some features rely on LSP settings (which the user can't configure until the rewrite).

@Elias-Ainsworth
Copy link
Author

I submitted a pr for the rebased commit. Should I close this one considering it may be unecessary?

@diniamo
Copy link
Collaborator

diniamo commented Nov 11, 2024 •
edited
Loading

You could've just pushed that to this branch, and set the target branch to v0.7 (which I would still prefer to preserve comments).

@Elias-Ainsworth Elias-Ainsworth changed the base branch from main to v0.7 November 12, 2024 20:51
@Elias-Ainsworth
Copy link
Author

You could've just pushed that to this branch, and set the target branch to v0.7 (which I would still prefer to preserve comments).

Sorry...I'm a little stupid

NotAShelf
NotAShelf requested changes Nov 15, 2024
View reviewed changes
flake.nix
Comment on lines +14 to +18
specialArgs = {
inherit lib;
};
}
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo formatting changes.

flake.nix
Comment on lines +72 to +77
packages = with pkgs; [
nil
statix
deadnix
alejandra
];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undo.

flake.nix Show resolved Hide resolved
modules/plugins/languages/nix.nix
package = pkgs.nixd;
internalFormatter = cfg.format.type == "nixpkgs-fmt";
lspConfig = ''
lspconfig.nixd.setup{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing on_attach.

modules/plugins/languages/nix.nix
Comment on lines +84 to +88
${
if (cfg.format.enable && cfg.format.type == "nixpkgs-fmt")
then useFormat
else noFormat
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Does nixd only support nixpkgs-fmt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NotAShelf NotAShelf NotAShelf requested changes

@diniamo diniamo Awaiting requested review from diniamo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Elias-Ainsworth @NotAShelf @diniamo