-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improved path handling for imports #127
Conversation
nixjs-rt/src/builtins.ts
Outdated
const pathValue = pathStrict.toJs(); | ||
let pathValue = ""; | ||
if (pathStrict instanceof NixString) { | ||
// TODO: Get relative paths working |
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.
Relative paths aren't supported in import when using strings. Here's an example:
% nix eval --impure --expr 'let flake = builtins.import "./flake.nix"; in flake.description'
error:
… while calling the 'import' builtin
at «string»:1:13:
1| let flake = builtins.import "./flake.nix"; in flake.description
| ^
… while realising the context of a path
at «none»:0: (source not available)
error: string './flake.nix' doesn't represent an absolute path
Maybe throw an error if the path is relative.
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.
Looks great, but I think importing relative paths as strings is not supported by nix, so can you please replace that part of the logic with an error (instead of that TODO)?
Hey yeah sorry for not getting the chance to address these, only got time to address 1 PR on the train before I went afk for a few days, will address in a day or two |
By the way does reviewing these feel easier or harder? Than just conventional large PRs |
I'm using plain github to review and it's pretty easy. Thanks for breaking these up! 👍 |
a90d9b1
to
631a2b5
Compare
This should allow easier testing of more obscure error cases. I might even replace other more rarely used errors with "other" because of this.
Slightly improved path handling for the import builtin, non-absolute string handling will be fixed in the future.
pathValue = normalizePath(pathStrict.toJs()); | ||
} else if (pathStrict instanceof Path) { | ||
pathValue = pathStrict.toJs(); | ||
} |
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.
Nitpick: you move the type exception from line 358 to here and remove the if
block above:
} | |
} else { | |
const expected = [Path, NixString]; | |
throw builtinBasicTypeMismatchError("import", pathStrict, expected); | |
} |
if (!isAbsolutePath(pathValue)) { | ||
throw otherError( | ||
err`string ${highlighted(JSON.stringify(pathValue))} doesn't represent an absolute path. Only absolute paths are allowed for imports.`, | ||
"builtins-import-non-absolute-path", |
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.
Nitpick: Maybe add a test for this? Feel free to ignore or add in a separate PR.
Improved path handling for imports
Slightly improved path handling for the import builtin, non-absolute string handling will be fixed in the future.
Stack created with Sapling. Best reviewed with ReviewStack.