numtide / treefmt-nix

treefmt nix configuration
https://numtide.github.io/treefmt/
MIT License
270 stars 83 forks source link

API feedback #2

Closed srid closed 1 year ago

srid commented 2 years ago

I tried using this as a flake-parts module. Relevant PRs,

Why is build an option?

In the first PR, you'll notice the following code:

              let mod = inputs.treefmt-nix.lib.evalModule pkgs { projectRootFile = "flake.nix"; };
              in builtins.removeAttrs mod.options [
                "_module"
                # TODO: Why is this an option if the user can't set it?
                "build"
              ];

This is the only way I could get access to the combined options (to pass to flake-parts module as is), but I was forced to hack around by removing the _module attribute, as well as build option. If I leave the build option around, the consumer of my flake-parts module is asked to explicitly set build.wrapper and friends (which doesn't make sense of course).

Why does the build option even exist if the user is not supposed to set them?

Also note that I'm forced to pass projectRootFile = "flake.nix";, so I'd think we may want to export the "final" options set in lib so consumers of this flake do not have to do all of the above. I'm not yet too familiar with the module system to come up with a cleaner approach as first attempt.

Accumulate formatter packages

This piece of snippet in the second PR is used to accumulate all formatter packages, along with the wrapper itself so that they can be put in the dev shell. This enables non-treefmt-aware editors to directly invoke the formatters. I imagine this can be upstreamed somehow here?

              treefmtConfig = (inputs.treefmt-nix.lib.evalModule pkgs config.treefmt).config;
              treefmtTools = {
                treefmt = treefmtConfig.build.wrapper;
              } // builtins.mapAttrs (_: v: v.package) treefmtConfig.programs;

programs and settings duality is unnecessary and problematic

I don't understand why this module provides two top-levels for configuring treefmt -- programs and settings. They even have overlapping options, ie.: programs.foo.package and programs.foo.command. I find that it is impossible to use both. For example, if you comment the multiline comment in the second PR it will lead to:

error: The option `perSystem.aarch64-darwin.treefmt.settings.formatter.ormolu.command' is used but not defined.
(use '--show-trace' to show detailed location information)
image

As an user, I'd expect something more straightforward like the following:

  settings.formatter.ormolu = {
    command = pkgs.haskellPackages.fourmolu;
    options = [
      "--ghc-opt"
      "-XImportQualifiedPost"
    ];
  };

We don't even need the enable flag.

zimbatm commented 2 years ago

Thanks, this is great feedback.

Why is build an option?

This is supposed to be internal. The main reason for having it in the tree is that it allows users to replace the results using the module system. For example, they could just link their repo's treefmt.toml and still get a wrapper out of it.

lib.evalModule is also the low-level API. I was expecting most users to use lib.mkWrapper instead.

Regarding projectRootFile, I think this could be a treefmt.toml option so that we could move the logic directly into the binary.

Accumulate formatter packages

Sounds good; the wrapper could have this set exposed in a passthru.

programs and settings duality is unnecessary and problematic

The settings attribute is supposed to be a 1:1 mapping with the treefmt.toml schema (plus the lib.getExe type extension). Because this repo is a collection of all the formatters we know of, a mechanism to turn them on/off is necessary. That's the main reason why both exist. But maybe there is a better way to arrange that?

EDIT: maybe "programs" is not the right term for these templated options. I mainly re-used the convention of NixOS and home-manager.

Other takeaways

One thing that's starting to appear is that the projectRootFile logic could be moved directly into treefmt. It might even become part of the treefmt.toml schema.

zimbatm commented 2 years ago

It might also make sense to merge the flake-parts module into this repo, if you want. Ideally in a way that doesn't explicitly add a flake dependency on flake-parts or nixpkgs.

srid commented 1 year ago

I'm happy with the API now that https://github.com/numtide/treefmt-nix/pull/14 is merged!