Skip to content

Adding ~user tilde recognition in file paths#5251

Merged
fdncred merged 29 commits intonushell:mainfrom
merelymyself:main
Apr 24, 2022
Merged

Adding ~user tilde recognition in file paths#5251
fdncred merged 29 commits intonushell:mainfrom
merelymyself:main

Conversation

@merelymyself
Copy link
Copy Markdown
Contributor

Description

Attempts to add ~user recognition for linux only.
Apologies for the previous PR. I swear clippy threw no errors when I ran it on my own machine...

Tests

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 --all --all-features -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo build; cargo test --all --all-features to check that all the tests pass

@merelymyself merelymyself marked this pull request as draft April 20, 2022 09:23
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 20, 2022

Can you please explain what this is supposed to do? I don't understand from your description. Also, maybe it's just me but, I really have an aversion to things that only work on one platform. At a minimum, one would think things like this would work on MacOS.

@merelymyself
Copy link
Copy Markdown
Contributor Author

I'm trying to make '~user' expand to the home path for the user in question... except it doesn't seem to be working at the moment. I'm not very experienced at this.

It should be working for MacOS without many changes, and I also hope to get it working for Windows soon.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 20, 2022

The ~user home completion is indeed missing and it would be a good addition, thanks for working on it!

Your code has a lot of unwraps and expects which are problematic in a shell which is supposed to never panic. It would be great if you could eliminate them. To me, it seems like in most cases you could work directly with Paths, or convert path.to_str() once beforehand and skip the ~user expansion if it fails.

Also, ideally, it should be cross-platform since we do not want Nushell to behave differently on different OSes.

@sophiajt
Copy link
Copy Markdown
Contributor

Yeah, I think this could be handy.

I was thinking - for crossplatform logic could we use the home path and then substitute the user's name for what is given after the ~? It'd be crossplatform and wouldn't pull in the additional dependency.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 20, 2022

@jntrnr Yeah, I was thinking the same but on Linux the root user has a different home path than /home/root. But maybe it's good enough or at least good enough on Windows.

…"/" - it was not relative. Removing the / makes it a relative path again.
@merelymyself merelymyself marked this pull request as ready for review April 21, 2022 08:24
@merelymyself merelymyself changed the title Adding ~user for linux Adding ~user tilde recognition in file paths Apr 21, 2022
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 21, 2022

How is this supposed to work?

I have 2 users on my Mac.

> ls /Users 
╭───┬────────────────┬──────┬───────┬────────────────╮
│ # │      name      │ type │ size  │    modified    │
├───┼────────────────┼──────┼───────┼────────────────┤
│ 0 │ /Users/Shared  │ dir  │   384 │ 3 days ago     │
│ 1 │ /Users/fdncred │ dir  │ 1,408 │ 35 minutes ago │
├───┼────────────────┼──────┼───────┼────────────────┤
│ # │      name      │ type │ size  │    modified    │
╰───┴────────────────┴──────┴───────┴────────────────╯

Am I supposed to be able to do ls ~/Shared or ls ~Shared or what? Also, is it supposed to work with tab completions? I'm so confused. LOL

@merelymyself
Copy link
Copy Markdown
Contributor Author

merelymyself commented Apr 21, 2022

ls ~Shared should be correct. Whenever ~user is used (say in a filepath such as ~user/foo/bar/txt.txt) it should automatically expand to the user's home dir. (In this case, /home/user/foo/bar/txt.txt or /Users/user/foo/bar/txt.txt or some equivalent.) I don't think it works with tab completions.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 21, 2022

ok, thanks for the clarification. i couldn't get ls ~Shared to work on my mac this morning.

@merelymyself
Copy link
Copy Markdown
Contributor Author

@fdncred - previously the function wasn't being called at all because ~user formats were being declined by the check if !path.starts_with("~"). ls ~user is working now for me.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 23, 2022

I still can't get this to work on Mac.
Screen Shot 2022-04-23 at 8 57 01 AM
It's probably true that there is no password for the Shared user. I'm just thinking that there's probably a better way to handle this than panicking and exiting nushell.

Changing this PR to draft until it's figured out.

@fdncred fdncred marked this pull request as draft April 23, 2022 13:58
@merelymyself
Copy link
Copy Markdown
Contributor Author

merelymyself commented Apr 23, 2022

Ahhhh, that is a problem I had not considered. I should probably match the error to just return /Users/username for MacOS.

ya, maybe instead of looking in the passwd file to get the home dir, maybe there's a better way to do it using a cross platform crate.

@kubouch
Copy link
Copy Markdown
Contributor

kubouch commented Apr 24, 2022

@merelymyself Instead of panicking, you can just return the un-expanded path.

@merelymyself merelymyself marked this pull request as ready for review April 24, 2022 15:10
@merelymyself
Copy link
Copy Markdown
Contributor Author

merelymyself commented Apr 24, 2022

@kubouch @fdncred thanks for the feedback! There's no chance of panicking now, I replaced all the .expect()s with options that wouldn't panic.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Apr 24, 2022

Seems like this is working now. Let's see if anything breaks. Thanks!

@fdncred fdncred merged commit b38f90d into nushell:main Apr 24, 2022
fennewald pushed a commit to fennewald/nushell that referenced this pull request Jun 27, 2022
* Added search terms to math commands

* Attempts to add ~user.

From: // Extend this to work with "~user" style of home paths

* Clippy recommendation

* clippy suggestions, again.

* fixing non-compilation on windows and macos

* fmt apparently does not like my imports

* even more clippy issues.

* less expect(), single conversion, match. Should work for MacOS too.

* Attempted to add functionality for windows: all it does is take the home path of current user, and replace the username.

* silly mistake in Windows version of user_home_dir()

* Update tilde.rs

* user_home_dir now returns a path instead of a string - should be smoother with no conversions to string

* clippy warnings

* clippy warnings 2

* Changed user_home_dir to return PathBuf now.

* Changed user_home_dir to return PathBuf now.

* forgot to fmt

* fixed windows build errors from modifying pathbuf but not returning it

* fixed windows clippy errors from returning () instead of pathbuf

* forgot to fmt

* borrowed path did not live long enough.

* previously, path.push did not work because rest_of_path started with "/" - it was not relative. Removing the / makes it a relative path again.

* Issue fixed.

* Update tilde.rs

* fmt.

* There is now a zero chance of panic. All expect()s have been removed.
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.

4 participants