Merge pull request #537 from cachix/simplify-before-after

Improve `before` and `after` internal logic

Author: domenkozar

modules/hook.nix

@@ -25,10 +25,39 @@ in
         '';
     };
 
+    id = mkOption {
+      type = types.str;
+      default = name;
+      defaultText = "the attribute name the hook submodule is bound to";
+      description =
+        ''
+          The unique identifier for the hook.
+
+          You do not need to set or modify this value.
+
+          The `id` is used to reference a hook when using `pre-commit run <id>`.
+          It can also be used to reference the hook in other hooks' `before` and `after` fields to define the order in which hooks run.
+
+          The `id` is set to the attribute name the hook submodule is bound to in the parent module.
+          For example, the `id` of following hook would be `my-hook`.
+
+          ```nix
+          {
+            hooks = {
+              my-hook = {
+                enable = true;
+                entry = "my-hook";
+              };
+            }
+          }
+          ```
+        '';
+    };
+
     name = mkOption {
       type = types.str;
-      defaultText = lib.literalMD "internal name, same as `id`";
       default = name;
+      defaultText = lib.literalMD "the attribute name the hook submodule is bound to, same as `id`";
       description =
         ''
           The name of the hook. Shown during hook execution.
@@ -202,14 +231,12 @@ in
         '';
       default = [ ];
     };
-
   };
 
   config = {
     raw =
       {
-        inherit (config) name entry language files types types_or exclude_types pass_filenames fail_fast require_serial stages verbose always_run args before after;
-        id = config.name;
+        inherit (config) id name entry language files types types_or exclude_types pass_filenames fail_fast require_serial stages verbose always_run args;
         exclude = mergeExcludes config.excludes;
       };
   };

modules/pre-commit.nix

@@ -9,7 +9,6 @@ let
     mapAttrsToList
     mkOption
     types
-    removeAttrs
     remove
     ;
 
@@ -34,25 +33,34 @@ let
     let
       sortedHooks = lib.toposort
         (a: b: builtins.elem b.id a.before || builtins.elem a.id b.after)
-        (mapAttrsToList
-          (id: value:
-            value.raw // {
-              inherit id;
-              before = value.raw.before;
-              after = value.raw.after;
-            }
-          )
-          enabledHooks
-        );
-      cleanedHooks = builtins.map (
-        hook:
-        removeAttrs hook [
-          "before"
-          "after"
-        ]
-      ) sortedHooks.result;
+        (builtins.attrValues enabledHooks);
     in
-    cleanedHooks;
+    if sortedHooks ? result then
+      builtins.map (value: value.raw) sortedHooks.result
+    else
+      let
+        getIds = builtins.map (value: value.id);
+
+        prettyPrintCycle = opts: cycle:
+          lib.pipe cycle [
+            (builtins.map (hook:
+              lib.nameValuePair hook.id { before = hook.before; after = hook.after; }
+            ))
+            lib.listToAttrs
+            (lib.generators.toPretty opts)
+          ];
+      in
+      throw ''
+        The hooks can't be sorted because of a cycle in the dependency graph:
+
+          ${concatStringsSep " -> " (getIds sortedHooks.cycle)}
+
+          which leads to a loop back to: ${concatStringsSep ", " (getIds sortedHooks.loops)}
+
+        Try removing the conflicting hook ids from the `before` and `after` attributes of these hooks:
+
+          ${prettyPrintCycle { indent = "  "; } sortedHooks.cycle}
+      '';
 
   configFile =
     performAssertions (