Skip to content
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.subpath.components: init #242695

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

infinisil
Copy link
Member

Description of changes

Adds a new function:

  • lib.path.subpath.components: Split a subpath into its path component strings.
    subpath.components "./foo//bar/./baz/"
    -> [ "foo" "bar" "baz" ]

Partly motivated by feedback in #238013. This is also part of the path library effort.

This work is sponsored by Antithesis

Things done
  • Documentation
  • Tests

@infinisil infinisil requested a review from edolstra as a code owner July 10, 2023 19:21
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Jul 10, 2023
@infinisil infinisil mentioned this pull request Jul 10, 2023
13 tasks
@infinisil infinisil changed the title lib.path.subpath.components: init lib.path.subpath.components: init Jul 10, 2023
@infinisil infinisil mentioned this pull request Jul 10, 2023
2 tasks
@@ -336,6 +336,34 @@ in /* No rec! Add dependencies on this file at the top. */ {
${subpathInvalidReason path}''
) 0 subpaths;

/* Split a subpath into its path component strings.
Throw an error if the subpath isn't valid, see `lib.path.subpath.isValid`.
Note that the returned path component strings are not valid subpaths themselves.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit unfortunate, because we decided to always prefix subpaths with ./: https://github.com/NixOS/nixpkgs/tree/master/lib/path#leading-dots-for-relative-paths

This is the only reason why subpaths are not valid as components.

Anyways, it's already too late now, and it's not like a big problem, just a bit unfortunate 🙃

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even think it's unfortunate. Those are clearly components, not subpaths. No ambiguity there, no inconsistency.

Copy link
Member Author

@infinisil infinisil Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this has a mistake in it. The resulting strings are valid subpaths, they're just not normalised.

Edit: Now reworded

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 10, 2023
@infinisil infinisil force-pushed the lib.path.subpath.components branch from 82560f0 to 89cf3e1 Compare July 19, 2023 14:29
@infinisil infinisil mentioned this pull request Jul 19, 2023
7 tasks
@@ -336,6 +336,34 @@ in /* No rec! Add dependencies on this file at the top. */ {
${subpathInvalidReason path}''
) 0 subpaths;

/* Split a subpath into its path component strings.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here subpath should also be a link to a definition, but I'll merge this so you can do it in the other PR where I said the same.

Copy link
Member Author

@infinisil infinisil Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(for reference: #244358 (comment))

Let's conclude that thread first before merging here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now added a link to subpath.isValid, which after the above PR is merged will contain a definition for it. This PR is not dependent on that PR however.

@roberth
Copy link
Member

roberth commented Jul 19, 2023

@ofborg eval

Looks like it got stuck earlier.

@infinisil infinisil mentioned this pull request Jul 19, 2023
2 tasks
@infinisil infinisil force-pushed the lib.path.subpath.components branch 3 times, most recently from 713651f to 109502d Compare July 20, 2023 20:36
Comment on lines +190 to +191
### Prefer returning subpaths over components
[subpath-preference]: #prefer-returning-subpaths-over-components
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's why this was added: #244358 (comment)

lib/path/README.md Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
subpath.join (subpath.components s) == subpath.normalise s

Type:
subpath.components :: String -> [ String ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
subpath.components :: String -> [ String ]
subpath.components :: String -> List String

I wish we didn't repeat Haskell's mistake. At your discretion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of lib.lists uses [ ] so let's stick with that for now. Am wondering what the problem with [ ] in Haskell is though (I only know Idris doesn't support [ ])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, it's a problem with dependent types, which hints at the more general problem that the list type is not a list.
In Nix we can exemplify this by asking the question what is [ Attrs String ]?
a. A type for lists of any number of elements, each of which is an attrset with string values only,
b1. A type for two-element heterogeneous lists where the first is an attrset and the second is a string,
b2. A type for one-element lists where the element is an attrset with string values only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That just looks like ambiguous notation to me, same as if you wrote List Attrs String. Needs parenthesis to make sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the b1/b2 distinction was a distraction. a/b2 is the one that's the problem: the list type is not a list, so it shouldn't use the notation of the list literal.

Copy link
Member Author

@infinisil infinisil Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I get it now, if both list values and types use the same syntax it's ambiguous and confusing. [ x ] could either be a single-value list with the element x, or if x is a type the type of a list of x's. In comparison [ x y ] can only be one.

So using List x for the type and [ ... ] for values disambiguates them better

infinisil and others added 2 commits July 26, 2023 23:29
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@infinisil infinisil force-pushed the lib.path.subpath.components branch from 9cfa604 to 407db58 Compare July 26, 2023 21:30
@infinisil
Copy link
Member Author

I think this is ready to get merged

@roberth roberth merged commit 8fa1697 into NixOS:master Aug 4, 2023
@infinisil infinisil deleted the lib.path.subpath.components branch August 4, 2023 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants