Add path primop. #1816
Merged
Add path primop. #1816
Conversation
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
|
Tested locally, will add tests and docs if this is deemed OK |
|
This is a new take on #1784, right? |
|
Yep! |
|
Ping |
|
@shlevy would you be able to write up a bit of docs for this function? |
|
If it's otherwise acceptable yeah |
edolstra
reviewed
Feb 6, 2018
src/libexpr/primops.cc
Outdated
| exists at an arbitrary path. */ | ||
| const auto path = expectedHash ? | ||
| path_ : | ||
| state.checkSourcePath(path_); |
There was a problem hiding this comment.
This looks wrong. In --restricted-eval, the path should be checked even if a hash is given. In --pure-eval, the path doesn't need to be checked but it requires a hash.
edolstra
reviewed
Feb 6, 2018
src/libexpr/primops.cc
Outdated
| @@ -2071,6 +2129,7 @@ void EvalState::createBaseEnv() | |||
| addPrimOp("__fromJSON", 1, prim_fromJSON); | |||
| addPrimOp("__toFile", 2, prim_toFile); | |||
| addPrimOp("__filterSource", 2, prim_filterSource); | |||
| addPrimOp("__addPath", 1, prim_addPath); | |||
There was a problem hiding this comment.
I'm not a fan of the name addPath (add to what?). Maybe it should just be builtins.path? That has a less imperative ring to it.
|
Looks good apart from 2 comments. |
builtins.path allows specifying the name of a path (which makes paths with store-illegal names now addable), allows adding paths with flat instead of recursive hashes, allows specifying a filter (so is a generalization of filterSource), and allows specifying an expected hash (enabling safe path adding in pure mode).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
buiiltins.path allows specifying the name of a path (which makes paths
with store-illegal names now addable), allows adding paths with flat
instead of recursive hashes, allows specifying a filter (so is a
generalization of filterSource), and allows specifying an expected
hash (enabling safe path adding in pure mode).