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.

Sign up for GitHub

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

Jump to bottom

lib.path.append: init #208887

Merged
merged 2 commits into from
Feb 7, 2023
Merged

lib.path.append: init #208887

merged 2 commits into from
Feb 7, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jan 3, 2023 •
edited
Loading

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

@infinisil infinisil requested a review from nbp as a code owner January 3, 2023 14:57
@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 Jan 3, 2023
@infinisil infinisil mentioned this pull request Jan 4, 2023
Merged
2 tasks
This was referenced Jan 4, 2023
Closed
Closed
roberth
roberth reviewed Jan 8, 2023
View reviewed changes
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Otherwise looks alright.

lib/path/default.nix Outdated Show resolved Hide resolved
@infinisil infinisil mentioned this pull request Jan 12, 2023
Closed
13 tasks
fricklerhandwerk
fricklerhandwerk approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

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

lib/path/default.nix Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
lib/path/default.nix Outdated Show resolved Hide resolved
@infinisil infinisil force-pushed the lib.path.append branch 2 times, most recently from 8b00eae to 50d1898 Compare January 17, 2023 17:40
@infinisil
Copy link
Member Author

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.

@infinisil infinisil mentioned this pull request Jan 17, 2023
Closed
2 tasks
@roberth
Copy link
Member

roberth commented Jan 17, 2023

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.

infinisil
infinisil commented Jan 18, 2023
View reviewed changes
lib/path/default.nix Outdated
*/
append = base: subpath:
assert assertMsg (isPath base) ''
lib.path.append: The first argument is of type ${builtins.typeOf base}, but a path was expected'';
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'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
Copy link
Member Author

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
Copy link
Member Author

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

@infinisil
lib.path: Minor improvements
5c4f616 
- 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
Copy link
Member Author

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

fricklerhandwerk
fricklerhandwerk approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

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

@infinisil
lib.path.append: init
eac2538 
This function can be used to append strings to Nix path values in a
safe way.
@infinisil
Copy link
Member Author

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

@infinisil
Copy link
Member Author

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

@infinisil infinisil merged commit a770c03 into NixOS:master Feb 7, 2023
@infinisil infinisil deleted the lib.path.append branch February 7, 2023 16:27
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Feb 8, 2023
@infinisil
lib.path: Minor improvements
d935001 
- 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)
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Feb 12, 2023
@infinisil
lib.path: Minor improvements
f70a9af 
- 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)
gador pushed a commit to gador/nixpkgs that referenced this pull request Feb 13, 2023
@infinisil @gador
lib.path: Minor improvements
253e3bd 
- 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 infinisil mentioned this pull request Feb 15, 2023
Open
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request May 31, 2023
@infinisil
lib.path: Minor improvements
9d338da 
- 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@fricklerhandwerk fricklerhandwerk fricklerhandwerk approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@infinisil @roberth @fricklerhandwerk