Utilize ShellName and Add Test to Enable Completions W/out Shell on Unix #48918
Conversation
I confirmed the proposal by Kalle, dotnet#48420 that the shellMap that gets provided is a default list of shell names like "fish" or "zsh" that are stored in the Shell Env Var. They are all lowercase in the shell map.
There was a problem hiding this comment.
Pull Request Overview
This PR aims to enable static completions to work in scenarios where a shell has not been specified on Unix by leveraging a default mapping of lowercase shell names. Key changes include:
- Adding a test (ShellDiscoveryTests.cs) to validate that the completions command works without an explicit shell.
- Updating CompletionsCommand.cs to convert the shell name to lowercase before lookup in the shell map.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/dotnet.Tests/CompletionTests/ShellDiscoveryTests.cs | Adds a new test ensuring completions work even without a specified shell. |
| src/System.CommandLine.StaticCompletions/CompletionsCommand.cs | Normalizes the shell name to lowercase to match against the shell map. |
|
|
||
| public class ShellDiscoveryTests() | ||
| { | ||
| [Fact] |
There was a problem hiding this comment.
maybe parameterize this test with the shell names and set the SHELL env var before each run so we can easily test all of our shell experiences??
There was a problem hiding this comment.
That's a great point -- I think this test should be ok since it works in general on any generic shell. If we had more capacity I would probably add this to the test, but not going to prioritize it right now. Thank you for reviewing
Co-authored-by: Copilot <[email protected]>
|
|
||
|
|
||
| using System.CommandLine.StaticCompletions.Shells; | ||
| using Microsoft.DotNet.Cli.Commands.BuildServer.Shutdown; |
There was a problem hiding this comment.
The using statement for Microsoft.DotNet.Cli.Commands.BuildServer.Shutdown appears unused in this test class. Unused using directives should be removed.
| using Microsoft.DotNet.Cli.Commands.BuildServer.Shutdown; |
|
|
||
| using System.CommandLine.StaticCompletions.Shells; | ||
| using Microsoft.DotNet.Cli.Commands.BuildServer.Shutdown; | ||
| using Microsoft.Extensions.Options; |
There was a problem hiding this comment.
The using statement for Microsoft.Extensions.Options appears unused in this test class. Unused using directives should be removed.
| using Microsoft.Extensions.Options; |
|
@copilot The new test used to work but now fails with: |
Co-authored-by: Copilot <[email protected]>
| var shellName = Path.GetFileName(shellPath); | ||
| if (shellMap.TryGetValue(shellPath, out var shellProvider)) | ||
| var shellName = Path.GetFileName(shellPath).ToLower(); | ||
| if (shellMap.TryGetValue(shellName, out var shellProvider)) |
There was a problem hiding this comment.
The .ToLower() call should not be necessary, because shellMap is using StringComparer.OrdinalIgnoreCase:
… unless you specifically want culture-specific case folding here, e.g. FISH to fısh in a Turkish culture. But that seems more likely to hurt than help because none of the known shell names includes a dotless ı.
| { | ||
| ParseResult result = Parser.Parse(["dotnet", "completions", "script"]); | ||
| result.Errors.Should().BeEmpty(); | ||
| result.GetValue<IShellProvider>("@CompletionsCommand_ShellArgument_Description").Should().NotBeNull(); |
There was a problem hiding this comment.
What is this at-sign syntax "@CompletionsCommand_ShellArgument_Description"? Shouldn't the argument be simply "shell", matching the Argument<IShellProvider> construction here:
3 participants
I confirmed the proposal by Kalle, which resolves #48420 and confirmed that the shellMap that gets provided is a default list of shell names like "fish" or "zsh" that are stored in the Shell Env Var. They are all lowercase in the shell map.