Evaluate string interpolation at parse time#11562
Conversation
|
so, any string interpolation is a const now? that sounds cool. people frequently ask for things like this. source $'($nu.default-config-dir)/some-file.nu'
use $'($some_constrcuted_dir)/a/path/to/file.nu' |
|
@fdncred Yeah, you'll be able to do that now. I realized after your comment that if you |
|
You're so fast 🔥 |
|
Since @kubouch worked so much on the |
|
Bravo |
|
Not really sure, if I put a sub command into string interpolation: Can it be handled at parse time? |
@WindSoilder I'm not sure which operations are recognized as constant but in your specific example the value isn't constant at parse time (must be written first). This PR makes only recognizes string interpolation as similar to |
|
@WindSoilder It can't be parse time because it tries to call an external command which is not supported at parse time. |
|
@ysthakur I think we should limit the string interpolation to only those cases where we wouldn't need The second issue is querying the config. To get config at runtime you need to pass the Stack, and we have a canonical way of doing it: nushell/crates/nu-engine/src/env.rs Line 355 in 90095c7 Stack as well to query the config, otherwise any local updates to $env.config won't be visible. We had a bunch of bugs related to this where engine_state.get_config() was used instead of the linked function and things like $env.config.xxx = yyy; command-that-uses-xxx-config wouldn't work. There are bugs like that probably still lurking around.
I guess you could create |
|
@kubouch Hmm, it'd be a bit tough to limit string interpolation to only non-filesize and non-datetime values. Perhaps Also, thanks for the tip on getting the config at runtime, I didn't know that existed. Cloning the config is unfortunate. I'll convert this back to a draft for now since it seems like it's going to take a bit of thinking |
|
There are so many value-to-string conversions now. It would be nice to consolidate at some point, not necessarily for this PR. |
|
I wouldn't block landing it because of the string conversions, we'd just need to document it. It would likely require messing with the string conversion code, and that would be better for another PR. The main thing is the Stack. I'm thinking that instead of cloning, the enum MaybeOwned {
Owned(Config)
Borrowed(&Config)
} |
|
Do you want me to make get_config return a MaybeOwned/Cow right in this PR or leave it for later? (I assume |
|
You can do it in this PR for the |
|
OK, looks good, let's land it! |
|
Great work @ysthakur! I'm loving this. Now we just have to rewrite our documentation that says these are not possible. And this one too const f = "foo.nu"
source $f |
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> Closes nushell#11561 # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> This PR will allow string interpolation at parse time. Since the actual config hasn't been loaded at parse time, this uses the `get_config()` method on `StateWorkingSet`. So file sizes and datetimes (I think those are the only things whose string representations depend on the config) may be formatted differently from how users have configured things, which may come as a surprise to some. It does seem unlikely that anyone would be formatting file sizes or date times at parse time. Still, something to think about if/before this PR merged. Also, I changed the `ModuleNotFound` error to include the name of the module. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> Users will be able to do stuff like: ```nu const x = [1 2 3] const y = $"foo($x)" // foo[1, 2, 3] ``` The main use case is `use`-ing and `source`-ing files at parse time: ```nu const file = "foo.nu" use $"($file)" ``` If the module isn't found, you'll see an error like this: ``` Error: nu::parser::module_not_found × Module not found. ╭─[entry nushell#3:1:1] 1 │ use $"($file)" · ─────┬──── · ╰── module foo.nu not found ╰──── help: module files and their paths must be available before your script is run as parsing occurs before anything is evaluated ``` # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --> Although there's user-facing changes, there's probably no need to change the docs since people probably already expect string interpolation to work at parse time. Edit: @kubouch pointed out that we'd need to document the fact that stuff like file sizes and datetimes won't get formatted according to user's runtime configs, so I'll make a PR to nushell.github.io after this one
<!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> Currently, in the test for interpolating strings at parse-time, the formatted string includes `(X years ago)` (from formatting a date) (test came from #11562). I didn't realize when I was writing it that it would have to be updated every year. This PR uses regex to check the output instead. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->
Closes #11561
Description
This PR will allow string interpolation at parse time.
Since the actual config hasn't been loaded at parse time, this uses the
get_config()method onStateWorkingSet. So file sizes and datetimes (I think those are the only things whose string representations depend on the config) may be formatted differently from how users have configured things, which may come as a surprise to some. It does seem unlikely that anyone would be formatting file sizes or date times at parse time. Still, something to think about if/before this PR merged.Also, I changed the
ModuleNotFounderror to include the name of the module.User-Facing Changes
Users will be able to do stuff like:
The main use case is
use-ing andsource-ing files at parse time:If the module isn't found, you'll see an error like this:
Tests + Formatting
After Submitting
Although there's user-facing changes, there's probably no need to change the docs since people probably already expect string interpolation to work at parse time.
Edit: @kubouch pointed out that we'd need to document the fact that stuff like file sizes and datetimes won't get formatted according to user's runtime configs, so I'll make a PR to nushell.github.io after this one