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

cp: expand target path before checking #11692

Merged
merged 3 commits into from Feb 1, 2024

Conversation

WindSoilder
Copy link
Collaborator

@WindSoilder WindSoilder commented Jan 31, 2024

Description

Fixes: #11683

User-Facing Changes

NaN

Tests + Formatting

I don't think we need to add a test, or else it'll copy some file to user's directory, it seems bad.
Done.

After Submitting

NaN

@bew
Copy link

bew commented Jan 31, 2024

I don't think we need to add a test, or else it'll copy some file to user's directory, it seems bad.

To write a test you could set $env.HOME to a test folder so actual user home isn't targeted?

@WindSoilder
Copy link
Collaborator Author

Thanks for your suggestion! Sadly I don't think it works. Because expand_path_with reads home dir from dirs_next::home_dir(), it doesn't know anything about $env.HOME

@bew
Copy link

bew commented Jan 31, 2024

Well in the end getting the home dir will look for the HOME env var (at least on Linux), so it should be possible to to set that env var before running the tests to ensure it doesn't leak into the test environment

https://doc.rust-lang.org/std/env/fn.home_dir.html

@WindSoilder
Copy link
Collaborator Author

Thanks! I've add a test on unix like system

@WindSoilder WindSoilder merged commit 16f3d9b into nushell:main Feb 1, 2024
19 checks passed
@hustcer hustcer added this to the v0.90.0 milestone Feb 3, 2024
@hustcer hustcer added the pr:bugfix This PR fixes some bug label Feb 3, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
Fixes: nushell#11683

# User-Facing Changes
NaN

# Tests + Formatting
~~I don't think we need to add a test, or else it'll copy some file to
user's directory, it seems bad.~~
Done.

# After Submitting
NaN
@WindSoilder WindSoilder deleted the cp_expand_path branch February 27, 2024 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:bugfix This PR fixes some bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cp $file ~/path/ gives is not a directory
3 participants