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

Improved path handling for imports #127

Merged
merged 2 commits into from
May 26, 2024
Merged

Improved path handling for imports #127

merged 2 commits into from
May 26, 2024

Conversation

arduano
Copy link
Collaborator

@arduano arduano commented May 9, 2024

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.

const pathValue = pathStrict.toJs();
let pathValue = "";
if (pathStrict instanceof NixString) {
// TODO: Get relative paths working
Copy link
Owner

@urbas urbas May 14, 2024

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.

Copy link
Owner

@urbas urbas left a 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)?

@arduano
Copy link
Collaborator Author

arduano commented May 15, 2024

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

@arduano
Copy link
Collaborator Author

arduano commented May 15, 2024

By the way does reviewing these feel easier or harder? Than just conventional large PRs

@urbas
Copy link
Owner

urbas commented May 15, 2024

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! 👍

@arduano arduano force-pushed the pr127 branch 2 times, most recently from a90d9b1 to 631a2b5 Compare May 20, 2024 10:30
@arduano arduano changed the title Improved path handling for imports (WIP) Improved path handling for imports May 20, 2024
arduano and others added 2 commits May 20, 2024 20:53
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();
}
Copy link
Owner

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:

Suggested change
}
} 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",
Copy link
Owner

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.

@urbas urbas merged commit b824a5c into master May 26, 2024
3 checks passed
@urbas urbas deleted the pr127 branch May 26, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants