Skip to content

Conversation

@eatradish
Copy link
Contributor

if input starts with '~' will complete to /home/USER

…support

if input starts with '~' will complete to `/home/USER`
Comment on lines +282 to +283
#[cfg(unix)]
if current.starts_with("~") {
Copy link
Member

Choose a reason for hiding this comment

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

This is marked as unix-only but the dep is always added

However, should we support this on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this feature should be supported on Windows, as it does not seem to be used on Windows.

Comment on lines +282 to +283
#[cfg(unix)]
if current.starts_with("~") {
Copy link
Member

Choose a reason for hiding this comment

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

Technically, there are other variants of ~ expansion. Should we support that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about what they are, can you explain?


pub(crate) fn complete_path(
value_os: &OsStr,
value_os: &str,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we changing this to be more restrictive?

Copy link
Contributor Author

@eatradish eatradish May 11, 2025

Choose a reason for hiding this comment

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

There doesn't seem to be a way for me to get &OsStr directly from String, maybe I should? (The reason for the change to String is that the replace_range method is only available for String)

let mut new_current = OsString::new();
new_current.write_str(&current).ok();

let mut candidates = complete_path(&new_current, current_dir, filter);

Comment on lines +278 to +280
let Some(current) = current.to_str() else {
return vec![];
};
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be doing this

Copy link
Contributor Author

@eatradish eatradish May 11, 2025

Choose a reason for hiding this comment

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

Maybe I should have used let mut current = current.to_string_lossy().to_string(); ?

shlex = { version = "1.3.0", optional = true }
completest = { version = "0.4.2", optional = true }
completest-pty = { version = "0.5.5", optional = true }
home = { version = "0.5.0", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

With std::env::home_dir being undeprecated, we should be using this through a polyfll. I'll need to go create one before this can move forward.

Copy link
Member

Choose a reason for hiding this comment

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

(if it wasn't for that, I'd be hesitant to add this out of question of whether the extra dep is worth it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epage
Copy link
Member

epage commented Aug 26, 2025

I'm sorry, I forgot about this PR when this came up in #6069 and we merged #6094 for this.

@epage epage closed this Aug 26, 2025
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.

2 participants