astro / deadnix

Scan Nix files for dead code
GNU General Public License v3.0
461 stars 15 forks source link

JSON as an alternative output format #14

Closed ilkecan closed 2 years ago

ilkecan commented 2 years ago

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 commented 2 years ago

Fine by me

water-sucks commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

water-sucks commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

astro commented 2 years ago

@Artturin That is the case since https://github.com/astro/deadnix/commit/9617e4c6ecbe96ce9b451ec0ee7b0951812decd1 I have released 0.1.5 which is merged in nixpkgs.