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

Add NU config to allow user be able to turn off external completion #5773

Merged
merged 18 commits into from
Jun 14, 2022

Conversation

Kangaxx-0
Copy link
Contributor

Description

Fix #5772, add an env var to let user be able to turn off external completion, this will be handy for WSL2 specifically, or PATH has tons of dirs

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

@elferherrera
Copy link
Contributor

How come this is not a config value instead of an env variable?

@Kangaxx-0
Copy link
Contributor Author

How come this is not a config value instead of an env variable?

You are correct, config makes more sense, I will make a quick update. Thank you for the comment

@Kangaxx-0 Kangaxx-0 changed the title Add NU env var to allow user be able to turn off external completion Add NU config to allow user be able to turn off external completion Jun 14, 2022
@@ -219,6 +219,7 @@ let-env config = {
disable_table_indexes: false # set to true to remove the index column from tables
cd_with_abbreviations: false # set to true to allow you to do things like cd s/o/f and nushell expand it to cd some/other/folder
case_sensitive_completions: false # set to true to enable case-sensitive completions
enable_external_completion: true # set to false to prevent nushell looks into $env.PATH and find more suggestions, recommended for WSL users
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enable_external_completion: true # set to false to prevent nushell looks into $env.PATH and find more suggestions, recommended for WSL users
enable_external_completion: true # set to false to prevent nushell looking into $env.PATH to find more suggestions. `false` recommended for WSL users as this look up may be very slow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment!

@fdncred
Copy link
Collaborator

fdncred commented Jun 14, 2022

As I commented in #5772, this is a known issue with WSL. I'm not sure we should add this when WSL is the problem and there is a known work-around already.

# in /etc/wsl.conf
[interop]
appendWindowsPath=false
# then run: `wsl --shutdown` and restart WSL

My work-around is to use the appendWindowsPath=false work-around and then manually add a couple paths to my WSL path for searching. For instance, I have to have the path to vscode in my path so that I can do code . and it just works.

I'm assuming that this just disables windows paths for completions but you can still call programs that are in your windows paths, like explorer and code, right?

@Kangaxx-0
Copy link
Contributor Author

As I commented in #5772, this is a known issue with WSL. I'm not sure we should add this when WSL is the problem and there is a known work-around already.

# in /etc/wsl.conf
[interop]
appendWindowsPath=false
# then run: `wsl --shutdown` and restart WSL

My work-around is to use the appendWindowsPath=false work-around and then manually add a couple paths to my WSL path for searching. For instance, I have to have the path to vscode in my path so that I can do code . and it just works.

I'm assuming that this just disables windows paths for completions but you can still call programs that are in your windows paths, like explorer and code, right?

Yes, that's right, this is just for completions. Also, from end user's standpoint, I feel like we can offer user a better option to show less suggested completions (only from Nu). Say if I have dozens of DIRS in my env PATH, user might go through a long list to find the command

@fdncred
Copy link
Collaborator

fdncred commented Jun 14, 2022

I'm still on the fence about whether this is a good idea or not.

  1. How would I enable completions for only certain windows folders? I guess you'd have to use the WSL solution (appendWindowsPaths = false) and enable_external_completion = true, and have certain folders in your nushell path that i want to complete from. Close to the same solution as without this change?
  2. We've had people complain about nushell not completing items in the path. This might make that more difficult. At least the first question is, do you have enable_external_completion enabled or disabled, I guess.
  3. A point for it is, I'm not sure how many people care about completing items in your path. I ususally don't use completions like that myself.

@Kangaxx-0
Copy link
Contributor Author

I'm still on the fence about whether this is a good idea or not.

  1. How would I enable completions for only certain windows folders? I guess you'd have to use the WSL solution (appendWindowsPaths = false) and enable_external_completion = true, and have certain folders in your nushell path that i want to complete from. Close to the same solution as without this change?
  2. We've had people complain about nushell not completing items in the path. This might make that more difficult. At least the first question is, do you have enable_external_completion enabled or disabled, I guess.
  3. A point for it is, I'm not sure how many people care about completing items in your path. I ususally don't use completions like that myself.

I understood your concern, however, I feel like this is more than WSL itself, e.g: regards to the bullet 2, I personally do not want to see any completion outside of nu, and I believe this is not only my preference :), instead, we offer our user an option which they could turn it off if, this should be a legitimate reason.

Also, the default flag value is true, uses won't feel any differences until they really see the slowness(PATH has tons of DIRS) and want to poke around the configuration, find out that config, eventually, turn if off and happy ending :)

If you guys feel like this is not the good change at this moment, I am ok with that decision too :)

@fdncred
Copy link
Collaborator

fdncred commented Jun 14, 2022

ok. let's try it out. thanks

@fdncred fdncred merged commit 534e1fc into nushell:main Jun 14, 2022
fennewald pushed a commit to fennewald/nushell that referenced this pull request Jun 27, 2022
…ushell#5773)

* 06-07-wsl

* 06-07-linux-issue-with-delete-input

* 06-08-2023

* 06-08-Linux

* commit for merge

* Fix unit test

* format

* clean code

* Add flag to turn off external completion

* change env var to config

* Fix comment

Co-authored-by: Frank Zhang <v-frankz@microsoft.com>
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.

External completion could cause a significant slowness
5 participants