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

JSON as an alternative output format #14

Closed
ilkecan opened this issue Feb 4, 2022 · 11 comments · Fixed by #16
Closed

JSON as an alternative output format #14

ilkecan opened this issue Feb 4, 2022 · 11 comments · Fixed by #16

Comments

@ilkecan
Copy link
Contributor

ilkecan commented Feb 4, 2022

Hi. How would you feel about adding an alternative output format that is easier to parse (like JSON)? I am interested in this to integrate deadnix into my editor using null-ls.vim. Would you accept such a PR?

@astro
Copy link
Owner

astro commented Feb 6, 2022

Fine by me

@water-sucks
Copy link

Just wondering, why is this an optional feature not enabled by default unless you compile from source manually? I would like to add it to my NixOS configuration to use with null-ls but the version in nixpkgs is 0.1.3 (without the JSON feature needed), and I attempted to use the flake but the feature flag was not enabled by default.

I'd like to know if there is a way to add this feature flag by overriding naersk-lib.buildPackage somehow in my own configuration flake (I've searched and haven't found any solutions till now) or if it should be enabled by default (I can make it a PR).

@ilkecan
Copy link
Contributor Author

ilkecan commented Apr 1, 2022

Since this feature is not required for the core functionality and needs additional dependencies, I disabled it by default in the PR.

If it helps, this is how I am currently using it:

  deadnix = unstable.deadnix.overrideAttrs(oldAttrs: rec {
    src = pkgs.fetchFromGitHub {
      owner = "astro";
      repo = "deadnix";
      rev = "0f78b370f0aab6f9512fa243e7a9da595b44f63b";
      sha256 = "sha256-TCGbFi0VbooyBv9vyOt1qtBFmTsQXH8sPOdMh9agMGU=";
    };
    cargoBuildFeatures = oldAttrs.cargoBuildFeatures  ++ [ "json-out" ];
    cargoDeps = oldAttrs.cargoDeps.overrideAttrs (_oldAttrs: {
      inherit src;
      outputHash = "sha256-A3i8YfmoFkcI/uUbaOIv4oCqnIqIiZtMH0ksOE2SSmk=";
    });
  });

@astro
Copy link
Owner

astro commented Apr 1, 2022

nixpkgs needs a deadnix version bump anyway. Should we enable the feature by default when using deadnix from nixpkgs?

@water-sucks
Copy link

Even if deadnix itself isn't exactly a language server, I think it's a good idea to add it to the default features list so people who want to use it as an LSP source can do it without using a overlay.

I used the code and it worked, but it just seems a little inconvenient because null-ls spits out a weird error if the JSON output isn't enabled and people don't know where to look.

@ilkecan
Copy link
Contributor Author

ilkecan commented Apr 1, 2022

To be clear, I would also prefer it being enabled by default in Nixpkgs. What I meant was, I didn't want to change the default behaviour myself. I think that decision should be made by Astro.

@astro
Copy link
Owner

astro commented Apr 1, 2022

@ilkecan I don't have a strong opinion on this myself, so let's enable the feature by default.

@water-sucks I'd love to add LSP functionality one day. On the other hand, editors talk to one LSP server only, right? Then it would make sense to integrate this with rnix-lsp.

@ilkecan
Copy link
Contributor Author

ilkecan commented Apr 1, 2022

The built-in Neovim LSP client can talk with multiple servers at the same time and this seems to also be the case for some other clients:

@water-sucks
Copy link

Depends on the editor in question. In Neovim you definitely can attach multiple language servers to a buffer; that's the way null-ls works in tandem with rnix and whatever other language server you configure.

I can try adding LSP capability to deadnix myself because it already has rnix as a dependency, though I'm not an expert in Rust so it could take a bit of time for me to do.

@Artturin
Copy link
Contributor

it would be good if deadnix was always compiled with this..

@astro
Copy link
Owner

astro commented Apr 12, 2022

@Artturin That is the case since 9617e4c I have released 0.1.5 which is merged in nixpkgs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants