Skip to content

Ensure currently_parsed_cwd is set for config files#12338

Merged
fdncred merged 2 commits intonushell:mainfrom
texastoland:relative-source-in-config
Apr 9, 2024
Merged

Ensure currently_parsed_cwd is set for config files#12338
fdncred merged 2 commits intonushell:mainfrom
texastoland:relative-source-in-config

Conversation

@texastoland
Copy link
Copy Markdown
Contributor

@texastoland texastoland commented Mar 30, 2024

Description

Fixes #7849, #11465 based on @kubouch's suggestion in #11465 (comment).

User-Facing Changes

Can source files relative to env.nu or config.nu like in #6150.

Tests + Formatting

Adds test that previously failed.

After Submitting

Fixes #7849, #11465. Adds test that previously failed.
@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Mar 31, 2024

I think after parsing the file, the directory should be restored back. The start_in_file() might have not been the best API choice when I implemented it… Probably the best way would be to use the same manual technique inside eval_source as in #6150 https://github.com/nushell/nushell/pull/6150/files#diff-9447a50446b532e347d2723b0e00c4388244964e6a0f57525fae2ec9ba52bab8R123

@texastoland
Copy link
Copy Markdown
Contributor Author

I think after parsing the file, the directory should be restored back

I added restoring the previous value.

The start_in_file() might have not been the best API choice

I prefer any kind of abstraction vs repeating the same pattern in numerous places. 1 option could be enter_script and exit_script maintaining a stack similar to enter_scope and exit_scope. I'm partial to process_script (or other name) accepting a closure to save the CWD before and restore it afterward.

the best way would be to use the same manual technique inside eval_source

That said I can remove start_in_file completely if you prefer.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 1, 2024

Looks good now. We can merge it after the release.

I just noticed the start_in_file() might be OK as it's used only by the EngineState and in scenarios where it won't be reused.

If you want to continue improving the API, you can continue here or open a new PR, just note that there is also #12262 which changes the API significantly, and hopefully, we'll manage to land it.

@texastoland
Copy link
Copy Markdown
Contributor Author

texastoland commented Apr 1, 2024

just note that there is also #12262 which changes the API significantly,

It looks to implement part of this suggestion:

maintaining a stack similar to enter_scope and exit_scope.

and hopefully, we'll manage to land it.

Yeah IFL it supersedes this so converting to draft until I can rebase on it.

If you want to continue improving the API, you can continue here

Any changes would be minimal so I'll lump them here 👌🏼

Thanks for the tips!

@texastoland texastoland marked this pull request as draft April 1, 2024 18:18
@texastoland texastoland marked this pull request as ready for review April 7, 2024 02:13
@fdncred fdncred merged commit 6536fa5 into nushell:main Apr 9, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 9, 2024

Thanks!

@hustcer hustcer added this to the v0.93.0 milestone Apr 9, 2024
FilipAndersson245 pushed a commit to FilipAndersson245/nushell that referenced this pull request Apr 10, 2024
<!--
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.
-->

Fixes nushell#7849, nushell#11465 based on @kubouch's suggestion in
nushell#11465 (comment).

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Can source files relative to `env.nu` or `config.nu` like in nushell#6150.

# 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
> ```
-->

Adds test that previously failed.

# 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.
-->
NotTheDr01ds pushed a commit to NotTheDr01ds/nushell that referenced this pull request Apr 12, 2024
<!--
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.
-->

Fixes nushell#7849, nushell#11465 based on @kubouch's suggestion in
nushell#11465 (comment).

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

Can source files relative to `env.nu` or `config.nu` like in nushell#6150.

# 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
> ```
-->

Adds test that previously failed.

# 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.
-->
@yizhepku
Copy link
Copy Markdown
Contributor

I prefer any kind of abstraction vs repeating the same pattern in numerous places.

Interesting. My position is that a bad abstraction is worse than no abstraction, and repeating the same pattern a few times is OK. This is why I chose to get rid of start_in_file() completely in my PR, because I think the name is really confusing and it did not see consistent usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

While starting nu the config.nu doesn't seem to be able to source any relative files.

5 participants