From 5c4f6161981917504c50b84864dcfd2f67dc037d Mon Sep 17 00:00:00 2001
From: Silvan Mosberger <silvan.mosberger@tweag.io>
Date: Wed, 18 Jan 2023 18:06:21 +0100
Subject: [PATCH] lib.path: Minor improvements

- Use isValid when possible instead of subpathInvalidReason: https://github.com/NixOS/nixpkgs/pull/209099#discussion_r1068714681
- Add documentation to function arguments
- Use newlines for error messages: https://github.com/NixOS/nixpkgs/pull/208887#discussion_r1069737602
- Add short comments for the unit test groups: https://github.com/NixOS/nixpkgs/pull/208887#discussion_r1072913051
- Slight formatting improvement for laws: https://github.com/NixOS/nixpkgs/pull/209099#discussion_r1068707955
---
 lib/path/default.nix    | 23 ++++++++++++++++-------
 lib/path/tests/unit.nix |  4 ++++
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/lib/path/default.nix b/lib/path/default.nix
index 96a9244407bf..6fa7c1dd6419 100644
--- a/lib/path/default.nix
+++ b/lib/path/default.nix
@@ -25,6 +25,10 @@ let
     assertMsg
     ;
 
+  inherit (lib.path.subpath)
+    isValid
+    ;
+
   # Return the reason why a subpath is invalid, or `null` if it's valid
   subpathInvalidReason = value:
     if ! isString value then
@@ -133,7 +137,9 @@ in /* No rec! Add dependencies on this file at the top. */ {
     subpath.isValid "./foo//bar/"
     => true
   */
-  subpath.isValid = value:
+  subpath.isValid =
+    # The value to check
+    value:
     subpathInvalidReason value == null;
 
 
@@ -150,11 +156,11 @@ in /* No rec! Add dependencies on this file at the top. */ {
 
   Laws:
 
-  - (Idempotency) Normalising multiple times gives the same result:
+  - Idempotency - normalising multiple times gives the same result:
 
         subpath.normalise (subpath.normalise p) == subpath.normalise p
 
-  - (Uniqueness) There's only a single normalisation for the paths that lead to the same file system node:
+  - Uniqueness - there's only a single normalisation for the paths that lead to the same file system node:
 
         subpath.normalise p != subpath.normalise q -> $(realpath ${p}) != $(realpath ${q})
 
@@ -210,9 +216,12 @@ in /* No rec! Add dependencies on this file at the top. */ {
     subpath.normalise "/foo"
     => <error>
   */
-  subpath.normalise = path:
-    assert assertMsg (subpathInvalidReason path == null)
-      "lib.path.subpath.normalise: Argument is not a valid subpath string: ${subpathInvalidReason path}";
-    joinRelPath (splitRelPath path);
+  subpath.normalise =
+    # The subpath string to normalise
+    subpath:
+    assert assertMsg (isValid subpath) ''
+      lib.path.subpath.normalise: Argument is not a valid subpath string:
+          ${subpathInvalidReason subpath}'';
+    joinRelPath (splitRelPath subpath);
 
 }
diff --git a/lib/path/tests/unit.nix b/lib/path/tests/unit.nix
index eccf3b7b1c33..da2c950de914 100644
--- a/lib/path/tests/unit.nix
+++ b/lib/path/tests/unit.nix
@@ -6,6 +6,7 @@ let
   inherit (lib.path) subpath;
 
   cases = lib.runTests {
+    # Test examples from the lib.path.subpath.isValid documentation
     testSubpathIsValidExample1 = {
       expr = subpath.isValid null;
       expected = false;
@@ -30,6 +31,7 @@ let
       expr = subpath.isValid "./foo//bar/";
       expected = true;
     };
+    # Some extra tests
     testSubpathIsValidTwoDotsEnd = {
       expr = subpath.isValid "foo/..";
       expected = false;
@@ -71,6 +73,7 @@ let
       expected = true;
     };
 
+    # Test examples from the lib.path.subpath.normalise documentation
     testSubpathNormaliseExample1 = {
       expr = subpath.normalise "foo//bar";
       expected = "./foo/bar";
@@ -107,6 +110,7 @@ let
       expr = (builtins.tryEval (subpath.normalise "/foo")).success;
       expected = false;
     };
+    # Some extra tests
     testSubpathNormaliseIsValidDots = {
       expr = subpath.normalise "./foo/.bar/.../baz...qux";
       expected = "./foo/.bar/.../baz...qux";