lib.path.append: init by infinisil · Pull Request #208887 · NixOS/nixpkgs

infinisil · 2023-01-03T14:57:41Z

Description of changes

This function can be used to append strings to Nix path values in a safe way, it's like <path> + ("/" + <string>) but safer. Relates to other work in the path library effort.

This work is sponsored by Antithesis ✨

Things done

Wrote documentation
Added and ran unit tests

roberth

Otherwise looks alright.

lib/path/default.nix

fricklerhandwerk

Just a few comments on cosmetics. Good stuff, and really compact thanks to a solid basis.

lib/path/default.nix

infinisil · 2023-01-17T17:44:01Z

Made some minor changes to the documentation. Using "path value type" when it could be ambiguous or being more precise is useful. And I'm not using any markdown syntax in the docs because it's not rendered at all. I looked into it a bit, but it's not very trivial to get it to render.

roberth · 2023-01-17T22:56:19Z

I'm not using any markdown syntax in the docs because it's not rendered at all.

I think it's fine to write markdown even if it's not rendered, but for now it really doesn't matter.

lib/path/tests/unit.nix

infinisil · 2023-01-18T15:31:26Z

lib/path/default.nix

I'm putting '' at the end of the string here and not on a newline because we don't want to print an extra newline (throw adds one). Apparently it doesn't matter though, because throw removes all trailing newlines anyways:

nix-repl> throw "hello\n\n\n\n\n" error: hello nix-repl>

I don't want to rely on that because it's undocumented and perhaps a bug. While I'm at it, there's a bug where throw "" doesn't print the red error: prefix:

nix-repl> throw "" nix-repl>

infinisil · 2023-01-18T15:50:50Z

I'm not using any markdown syntax in the docs because it's not rendered at all.

I think it's fine to write markdown even if it's not rendered, but for now it really doesn't matter.

I guess a good policy for now is: "Use markdown syntax when it doesn't make it significantly harder to read without being rendered". I'm using that here now

infinisil · 2023-01-18T17:10:26Z

Now consistently using isValid and newlines in errors for all path library functions now, also added argument documentation.

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment) - Slight formatting improvement for laws: NixOS#209099 (comment)

infinisil · 2023-01-18T17:25:20Z

Now even more consistency with the other PRs. I think this is ready to be merged

fricklerhandwerk

As always, amazing work. ✨ Thanks a lot for your patience with the reviews.

This function can be used to append strings to Nix path values in a safe way.

infinisil · 2023-01-24T17:34:30Z

@roberth Does it also look good to you? If so I'd appreciate a mergin :)

infinisil · 2023-02-07T16:26:08Z

I'll go ahead and merge this myself since @roberth mentioned already in the beginning that it looks good :)

- Use isValid when possible instead of subpathInvalidReason: NixOS/nixpkgs#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS/nixpkgs#208887 (comment) - Add short comments for the unit test groups: NixOS/nixpkgs#208887 (comment) - Slight formatting improvement for laws: NixOS/nixpkgs#209099 (comment)

- Use isValid when possible instead of subpathInvalidReason: NixOS#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS#208887 (comment) - Add short comments for the unit test groups: NixOS#208887 (comment) - Slight formatting improvement for laws: NixOS#209099 (comment)

- Use isValid when possible instead of subpathInvalidReason: NixOS/nixpkgs#209099 (comment) - Add documentation to function arguments - Use newlines for error messages: NixOS/nixpkgs#208887 (comment) - Add short comments for the unit test groups: NixOS/nixpkgs#208887 (comment) - Slight formatting improvement for laws: NixOS/nixpkgs#209099 (comment)

infinisil requested review from edolstra, fricklerhandwerk and roberth January 3, 2023 14:57

infinisil requested a review from nbp as a code owner January 3, 2023 14:57

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 3, 2023

infinisil mentioned this pull request Jan 4, 2023

lib.path.subpath.join: init #209099

Merged

2 tasks

infinisil force-pushed the lib.path.append branch from f3a0229 to 263735c Compare January 4, 2023 22:25

This was referenced Jan 4, 2023

Path library design #200718

Closed

lib.path.difference: init #209375

Closed

roberth reviewed Jan 8, 2023

View reviewed changes

lib/path/default.nix Outdated Show resolved Hide resolved

infinisil force-pushed the lib.path.append branch from 263735c to 680c1f0 Compare January 9, 2023 21:59

infinisil mentioned this pull request Jan 12, 2023

Path library functions #210426

Closed

13 tasks

fricklerhandwerk approved these changes Jan 13, 2023

View reviewed changes

infinisil force-pushed the lib.path.append branch 2 times, most recently from 8b00eae to 50d1898 Compare January 17, 2023 17:40

infinisil mentioned this pull request Jan 17, 2023

lib.path.hasPrefix, lib.path.removePrefix: init #210423

Closed

2 tasks

roberth reviewed Jan 17, 2023

View reviewed changes

lib/path/tests/unit.nix Outdated Show resolved Hide resolved

infinisil force-pushed the lib.path.append branch from 50d1898 to e4352f7 Compare January 18, 2023 15:27

infinisil commented Jan 18, 2023

View reviewed changes

infinisil force-pushed the lib.path.append branch from e4352f7 to 1c2587c Compare January 18, 2023 15:48

infinisil force-pushed the lib.path.append branch from 1c2587c to 312a8df Compare January 18, 2023 17:02

infinisil force-pushed the lib.path.append branch from 312a8df to b7f7c25 Compare January 18, 2023 17:10

infinisil force-pushed the lib.path.append branch from b7f7c25 to 3a10261 Compare January 18, 2023 17:17

infinisil force-pushed the lib.path.append branch from 3a10261 to e4fc836 Compare January 18, 2023 17:19

fricklerhandwerk approved these changes Jan 18, 2023

View reviewed changes

lib.path.append: init

eac2538

This function can be used to append strings to Nix path values in a safe way.

infinisil force-pushed the lib.path.append branch from e4fc836 to eac2538 Compare January 18, 2023 19:17

infinisil merged commit a770c03 into NixOS:master Feb 7, 2023

infinisil deleted the lib.path.append branch February 7, 2023 16:27

infinisil mentioned this pull request Feb 15, 2023

Tilde (~) in path literal is illegal NixOS/nix#7742

Open

Uh oh!

Comments

Conversation

infinisil commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fricklerhandwerk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

infinisil commented Jan 17, 2023

Uh oh!

roberth commented Jan 17, 2023

Uh oh!

Uh oh!

infinisil Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil commented Jan 18, 2023

Uh oh!

infinisil commented Jan 18, 2023

Uh oh!

infinisil commented Jan 18, 2023

Uh oh!

fricklerhandwerk left a comment

Choose a reason for hiding this comment

Uh oh!

infinisil commented Jan 24, 2023

Uh oh!

infinisil commented Feb 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

infinisil commented Jan 3, 2023 •

edited

Loading