Codecov Report

Merging #8094 (c80f9a8) into main (789b2e6) will increase coverage by 0.56%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##             main    #8094      +/-   ##
==========================================
+ Coverage   54.23%   54.79%   +0.56%     
==========================================
  Files         608      608              
  Lines       99003    99147     +144     
==========================================
+ Hits        53693    54331     +638     
+ Misses      45310    44816     -494     

@rgwood I'm not really sure what to look for in the failed CI since everything passes locally. Could this be a temporary issue or something that actually needs a fix?

I think you came across the flakiness of the plugin tests on the Mac CI runners. Restarted them, should typically resolve.

Thank you for taking your time. I've applied fixes according to your comments, they make sense to me and I see I'll have to check more of what Rust has already implemented to not do double work where applicable.

I still have to add the test, which I expect to do shortly.

@sholderbach I believe I've applied all the changes, including the test. Hopefully it captures what you meant.

Thanks for updating the code and adding the additional independent tests!
I think this addresses what I asked for.

I would hold off on merging for a moment as @kubouch is currently fighting with the completion logic a bit to track down a nasty panic or logic bug (#7779 #8102). We certainly need to reign in a bunch of kludgy special cases that already exist and make everything a bit brittle. If we get this cleared it is probably easier to say how your fix fits in.
So we probably have to wait until after the imminent release.

This maybe also gives us some time to clarify how sudo should behave in regards to internal and external commands (referring to your #8099)

Thanks for being responsive and eager to help out with nushell!

I'm OK with it, I was tracking mostly bugs related to aliases, this seems unrelated.

I would move find_non_whitespace_index() and is_sudo() away from the EngineState because the first does not reference EngineState at all and I'm not sure we want to hard-code sudo detection into the EngineState.

Another change I would do is to rename is_sudo() to something more generic, like is_passthrough_command() because tools like bear or gdb would benefit from the same autocomplete support as sudo. We can keep "sudo" hardcoded there for now and consider adding support for more commands later. When adding support for more command, we don't want to hard-code every single command into Nushell, so one way to do it would be to have an environment variable with a list of commands (populated by sudo by default) that the completer would look up.

Excellent suggestions, thank you. I'll take care of them tomorrow I think.

Instead of utils, I would put the two functions into the command_completer.rs because they're used only for the completions.

The logic for detecting the sudo is also a bit gnarly. The completer has access to the parsed result so there is no need to iterate bytes, you should be able to check for Expr::ExternalCall and see if it matches sudo and go from there. Or something like that. But I think as long as it works and does not break anything, it should be OK to merge as well.

This will benefit from some squashing before merging. Let me know if it's alright as it is.

We won't merge it at least for the next 1-2 days because we're doing the release, so if you want to keep hacking on it, go ahead. In general, looking up the bytes manually seems a bit unsound to me, I'd rather use the parsed output.

One more thing, though. You're using direct array access with [] which can panic. In Nushell, if something can panic, it will panic at some point. It would be better to use .get() or something similar that doesn't panic.

No worries, I'm not in a rush for having this merged, just saying when you feel it's ready, I'll also also squash the changes.

I've replaced [..] with .get, thanks for pointing it out to me.

I think I'm only using bytes where I don't have the full string available so that can probably stay like that? I'm perfectly fine with adapting the code if you point it out where you think I should do it differently.

This doesn't seem to allow completions to complete local paths (i.e. programs that are not in PATH).

Example:

You have a script in a folder that requires root privileges to run. It is not in the PATH environment variable.
If you are currently in your home directory, and the script is in ~/scripts/do_admin_stuff.nu, you could invoke it as sudo scripts/do_admin_stuff.nu or sudo ./scripts/do_admin_stuff.nu. Trying to complete either of these paths results in the message "NO RECORDS FOUND". Trying to define an extern to override this functionality also does not work.