Skip to content

auto-expand paths in the $nu variable#8653

Merged
fdncred merged 1 commit intonushell:mainfrom
fdncred:auto_expand_nu
Mar 29, 2023
Merged

auto-expand paths in the $nu variable#8653
fdncred merged 1 commit intonushell:mainfrom
fdncred:auto_expand_nu

Conversation

@fdncred
Copy link
Copy Markdown
Contributor

@fdncred fdncred commented Mar 28, 2023 •
edited
Loading

Description

I recently ran into an issue where one of the $nu paths was not expanded and was causing issue because $env.PWD did not equal $nu.temp-path, even though I was in $nu.temp-path. The reason it didn't match was because my temp path was symlinked. This PR fixes that issue by expanding the paths with canonicalize_with().

User-Facing 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 -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

Note
from nushell you can also use the toolkit as follows

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 after the PR is merged, if necessary. This will help us keep the docs up to date.

@fdncred
Copy link
Copy Markdown
Contributor Author

fdncred commented Mar 28, 2023

@amtoine - if this lands, it'll allow us to take some of the path expands off of the stdlib where it was failing previously on macos.

amtoine
amtoine approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amtoine - if this lands, it'll allow us to take some of the path expands off of the stdlib where it was failing previously on macos.

i won't be able to reproduce this i fear, 'cause i never had this issue on my linux machine...

but that looks great and yeah, for sure we can simplify the tests once it lands 👍

@fdncred fdncred merged commit a49e5b3 into nushell:main Mar 29, 2023
@fdncred fdncred deleted the auto_expand_nu branch March 29, 2023 18:24
amtoine added a commit to amtoine/nushell that referenced this pull request Mar 29, 2023
@amtoine
remove the redundant path expand from the tests
04ee3dc 
This should have been fixed in nushell#8653.
@amtoine amtoine mentioned this pull request Mar 29, 2023
Merged
fdncred pushed a commit that referenced this pull request Mar 29, 2023
@amtoine
REFACTOR: remove the redundant path expand from the tests of the st…
ed0fce9 
…andard library (#8666)

Related to #8653.

# Description
This PR removes the redundant `path expand`s introduced in #8552 and
#8576.

# User-Facing Changes
```
$nothing
```

# Tests + Formatting
the tests still pass on linux.

# After Submitting
```
$nothing
```
@rgwood
Copy link
Copy Markdown
Contributor

rgwood commented Mar 31, 2023

Looks like this breaks the tests::test_config_path::test_default_config_path test on any system with Nu config symlinked:

thread 'tests::test_config_path::test_default_config_path' panicked at 'assertion failed: `(left == right)`
  left: `"/home/reilly/dotfiles/nu/config.nu"`,
 right: `"/home/reilly/.config/nushell/config.nu"`', src/tests/test_config_path.rs:10:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/panicking.rs:65:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/panicking.rs:203:5
   4: nu::tests::test_config_path::test_default_config_path
             at ./src/tests/test_config_path.rs:10:5
   5: nu::tests::test_config_path::test_default_config_path::{{closure}}
             at ./src/tests/test_config_path.rs:4:1
   6: core::ops::function::FnOnce::call_once
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/ops/function.rs:251:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/ops/function.rs:251:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@fdncred fdncred mentioned this pull request Mar 31, 2023
Merged
fdncred added a commit that referenced this pull request Mar 31, 2023
@fdncred
fix test_default_config_path test after pr 8653 (#8690)
b2257a5 
# Description

This PR fixes a testing bug that @rgwood found and PR #8653 introduced,
mentioned
[here](#8653 (comment)).
Since 8653 returns canonicalizes config paths now, the tests need to
return canonicalized paths as well. Without this PR, if you have your
nushell config dir symlinked, this test will fail. This PR fixes that
test.

# User-Facing 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 -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- crates/nu-utils/standard_library/tests.nu` 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amtoine amtoine amtoine approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fdncred @rgwood @amtoine