-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
lib.path.difference: init #209375
Closed
Closed
lib.path.difference: init #209375
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
6e4768c
lib.path.difference: init
infinisil 5e5b9d8
lib.path.difference: Only trigger non-empty assert for commonAncestor
infinisil 47ace14
Address some review
infinisil 2debd69
go -> recurse
infinisil 8f094bc
Use newlines for multiple roots error
infinisil d469d68
Simplify code a bit and use prefix/suffix naming
infinisil 6fa6578
Write documentation
infinisil 44032d8
Add tests
infinisil 090b1f1
Address review
infinisil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,11 @@ let | |
inherit (builtins) | ||
isString | ||
isPath | ||
isAttrs | ||
dirOf | ||
baseNameOf | ||
typeOf | ||
seq | ||
split | ||
match | ||
; | ||
|
@@ -18,6 +23,8 @@ let | |
all | ||
concatMap | ||
foldl' | ||
take | ||
drop | ||
; | ||
|
||
inherit (lib.strings) | ||
|
@@ -33,10 +40,16 @@ let | |
isValid | ||
; | ||
|
||
inherit (lib.attrsets) | ||
mapAttrsToList | ||
listToAttrs | ||
nameValuePair | ||
; | ||
|
||
# Return the reason why a subpath is invalid, or `null` if it's valid | ||
subpathInvalidReason = value: | ||
if ! isString value then | ||
"The given value is of type ${builtins.typeOf value}, but a string was expected" | ||
"The given value is of type ${typeOf value}, but a string was expected" | ||
else if value == "" then | ||
"The given string is empty" | ||
else if substring 0 1 value == "/" then | ||
|
@@ -100,6 +113,16 @@ let | |
# An empty string is not a valid relative path, so we need to return a `.` when we have no components | ||
(if components == [] then "." else concatStringsSep "/" components); | ||
|
||
# Takes a Nix path value and deconstructs it into the filesystem root | ||
# (generally `/`) and a subpath | ||
deconstructPath = | ||
let | ||
recurse = components: path: | ||
# If the parent of a path is the path itself, then it's a filesystem root | ||
if path == dirOf path then { root = path; inherit components; } | ||
else recurse ([ (baseNameOf path) ] ++ components) (dirOf path); | ||
in recurse []; | ||
|
||
in /* No rec! Add dependencies on this file at the top. */ { | ||
|
||
/* Append a subpath string to a path. | ||
|
@@ -149,6 +172,120 @@ in /* No rec! Add dependencies on this file at the top. */ { | |
${subpathInvalidReason subpath}''; | ||
path + ("/" + subpath); | ||
|
||
/* Determine the difference between multiple paths, including the longest | ||
common prefix and the individual suffixes between them | ||
|
||
The input is an attribute set of paths, where the keys are only for naming | ||
and can be picked arbitrarily. The returned attribute set contains two | ||
attributes: | ||
infinisil marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- `commonPrefix`: A path value containing the common prefix between all the | ||
given paths. | ||
|
||
- `suffix`: An attribute set of normalised subpaths (see | ||
`lib.path.subpath.normalise`). The keys are the same | ||
as were given as the input, they can be used to easily match up the | ||
suffixes to the inputs. | ||
|
||
Throws an error if all paths don't share the same filesystem root. | ||
|
||
Laws: | ||
|
||
- The input paths can be recovered by appending each suffix to the common ancestor | ||
|
||
forall paths, result = difference paths. | ||
mapAttrs (_: append result.commonPrefix) result.suffix == paths | ||
|
||
- The _longest_ common prefix is returned | ||
|
||
forall paths, result = difference paths. | ||
! exists longerPrefix. hasProperPrefix result.commonPrefix longerPrefix && all (hasPrefix longerPrefix) (attrValues paths) | ||
Comment on lines
+199
to
+202
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this PR weakly depends on #210423 because of the docs here. The |
||
|
||
- Suffixes are normalised | ||
|
||
forall paths, result = difference paths. | ||
mapAttrs (_: subpath.normalise) result.suffix == result.suffix | ||
|
||
Type: | ||
difference :: AttrsOf Path -> { commonPrefix :: Path, suffix :: AttrsOf String } | ||
|
||
Example: | ||
difference { foo = ./foo; bar = ./bar; } | ||
=> { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./bar"; }; } | ||
|
||
difference { foo = ./foo; bar = ./.; } | ||
=> { commonPrefix = ./.; suffix = { foo = "./foo"; bar = "./."; }; } | ||
|
||
difference { foo = ./foo; bar = ./foo; } | ||
=> { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./."; }; } | ||
|
||
difference { foo = ./foo; bar = ./foo/bar; } | ||
=> { commonPrefix = ./foo; suffix = { foo = "./."; bar = "./bar"; }; } | ||
*/ | ||
difference = | ||
# The attribute set of paths to calculate the difference between | ||
paths: | ||
let | ||
# Deconstruct every path into its root and subpath | ||
deconstructedPaths = mapAttrsToList (name: value: | ||
# Check each item to be an actual path | ||
assert assertMsg (isPath value) "lib.path.difference: Attribute ${name} is of type ${typeOf value}, but a path was expected"; | ||
deconstructPath value // { inherit name value; } | ||
) paths; | ||
|
||
firstPath = | ||
assert assertMsg | ||
(paths != {}) | ||
"lib.path.difference: An empty attribute set was given, but a non-empty attribute set was expected"; | ||
head deconstructedPaths; | ||
|
||
# The common root to all paths, errors if there are different roots | ||
commonRoot = | ||
# Fast happy path in case all roots are the same | ||
if all (path: path.root == firstPath.root) deconstructedPaths then firstPath.root | ||
# Slower sad path when that's not the case and we need to throw an error | ||
else foldl' | ||
(skip: path: | ||
if path.root == firstPath.root then skip | ||
else throw '' | ||
lib.path.difference: Filesystem roots must be the same for all paths, but paths with different roots were given: | ||
${firstPath.name} = ${toString firstPath.value} (root ${toString firstPath.root}) | ||
${toString path.name} = ${toString path.value} (root ${toString path.root})'' | ||
) | ||
null | ||
deconstructedPaths; | ||
|
||
commonAncestorLength = | ||
let | ||
recurse = index: | ||
let firstComponent = elemAt firstPath.components index; in | ||
if all (path: | ||
# If all paths have another level of components | ||
length path.components > index | ||
# And they all match | ||
&& elemAt path.components index == firstComponent | ||
) deconstructedPaths | ||
then recurse (index + 1) | ||
else index; | ||
in | ||
# Ensure that we have a common root before trying to find a common ancestor | ||
# If we didn't do this one could evaluate `relativePaths` without an error even when there's no common root | ||
seq commonRoot (recurse 0); | ||
|
||
commonPrefix = commonRoot | ||
+ ("/" + joinRelPath (take commonAncestorLength firstPath.components)); | ||
|
||
suffix = listToAttrs (map (path: | ||
nameValuePair path.name (joinRelPath (drop commonAncestorLength path.components)) | ||
) deconstructedPaths); | ||
in | ||
assert assertMsg | ||
(isAttrs paths) | ||
"lib.path.difference: The given argument is of type ${typeOf paths}, but an attribute set was expected"; | ||
roberth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
inherit commonPrefix suffix; | ||
}; | ||
|
||
/* Whether a value is a valid subpath string. | ||
|
||
- The value is a string | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simpler, shorter name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
splitPath would also be a name for something like
strings.split "/"
splitPathFromRoot
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not just splitting it from the root but also splitting the path itself. Ultimately this is an internal variable name, it doesn't matter much as long as we don't expose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sure matters less than an exported name, but for readability (= maintainability) it's relevant nonetheless. The suggestion is merely to (slightly) reduce cognitive load of reading a long word with many syllables. It arguably eases readability by only a small amount, but we have a long leverage on the future here. In any case, I'm not pushing this one at all, just trying to keep things at the amazing level of quality that you set out with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not be held to this standard all the time, yes I'm a perfectionist, but at some level it's just not worth it. There will always be things that aren't immediately obvious from just the name, in which case programmers can read comments, the source code, run the code themselves, etc.
But it's also entirely subjective which name is best. In this case, I think
deconstructPath
is still the best name, and me being the original author of the code should give that opinion some weight.In particular, I also think
deconstructPath
being non-obvious is even a good thing, because what the function does is in fact non-obvious and non-trivial, I'd rather people look at the definition and comment instead of assuming they know what it means.