Crazy workload version tab-completion idea #2
Conversation
| }.AddCompletions((ctx) => | ||
| { | ||
| var versions = Search.Versions.SearchWorkloadSetsCommand.GetWorkloadSetVersions(ctx).GetAwaiter().GetResult(); | ||
| return versions.Select(v => new System.CommandLine.Completions.CompletionItem(v)); | ||
| }); |
There was a problem hiding this comment.
Here's the completions hookup - we needed something that could take the completion-time parse result and turn it into CompletionItems.
| @@ -60,8 +60,8 @@ private static CompletionItem[] Completions(ParseResult complete) | |||
|
|
|||
| var result = Parser.Instance.Parse(input); | |||
|
|
|||
| return result.GetCompletions(position) | |||
| .Distinct() | |||
| var completions = result.GetCompletions(position); | |||
There was a problem hiding this comment.
tiny change for making debugging easier!
| @@ -2,26 +2,17 @@ | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
|
|
|||
| using System.CommandLine; | |||
| using System.Text.Json; | |||
There was a problem hiding this comment.
Big changes to this class - I pulled out the code and related members for the code into a new SearchWorkloadSetsCommand to match the existing parser.
| protected override void ShowHelpOrErrorIfAppropriate(ParseResult parseResult) | ||
| { | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
This is an odd call-out - the CommandBase that WorkloadCommandBase inherits from unconditionally calls the 'bail out if necessary' extension method we have, but I think that's bad behavior. In the tab-completion case, you very well may get invalid/erroring parse results, and so throwing those errors in the completion context prevents completions from being shown.
| public async Task<int> Execute(CancellationToken cancellationToken) | ||
| { | ||
| var featureBand = new SdkFeatureBand(SdkVersion); | ||
| var packageId = Installer.GetManifestPackageId(WorkloadPackageIdBase, featureBand); | ||
| var versions = (await PackageDownloader.GetLatestPackageVersions(packageId, NumberOfWorkloadSetsToTake, packageSourceLocation: null, includePreview: !string.IsNullOrWhiteSpace(SdkVersion.Prerelease)).ConfigureAwait(false)) | ||
| .Select(version => WorkloadManifestUpdater.WorkloadSetPackageVersionToWorkloadSetVersion(featureBand, version.Version.ToString())); | ||
| if (_workloadSetOutputFormat == SearchWorkloadSetsFormat.json) | ||
| { | ||
| Reporter.WriteLine(JsonSerializer.Serialize(versions.Select(version => version.ToDictionary(_ => "workloadVersion", v => v)))); | ||
| } | ||
| else | ||
| { | ||
| Reporter.WriteLine(string.Join('\n', versions)); | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
This is all the same logic from the other class, just in this new class.
| public static async Task<IEnumerable<string>> GetWorkloadSetVersions(CompletionContext ctx) | ||
| { | ||
| var inner = new SearchWorkloadSetsCommand(ctx.ParseResult); | ||
| var featureBand = new SdkFeatureBand(inner.SdkVersion); | ||
| var packageId = inner.Installer.GetManifestPackageId(WorkloadPackageIdBase, featureBand); | ||
| var versions = (await inner.PackageDownloader.GetLatestPackageVersions(packageId, inner.NumberOfWorkloadSetsToTake, packageSourceLocation: null, includePreview: !string.IsNullOrWhiteSpace(inner.SdkVersion.Prerelease)).ConfigureAwait(false)) | ||
| .Select(version => WorkloadManifestUpdater.WorkloadSetPackageVersionToWorkloadSetVersion(featureBand, version.Version.ToString())); | ||
| return versions; | ||
| } |
There was a problem hiding this comment.
This is a simple static accessor to the same core logic in Execute, for tab completion purposes.
There was a problem hiding this comment.
I do wonder if we can make a helper method to reuse more of this; though most of the names of things are different, the logic is mostly the same
…version calculations
baronfel
force-pushed
the
refactor-workload-versions-for-completions
branch
from
2b9fa55
to
5b29956
Compare
| internal enum SearchWorkloadSetsFormat | ||
| { | ||
| list, | ||
| json | ||
| } |
There was a problem hiding this comment.
making this an enum makes the help display look nicer, and also makes the code a nicer comparison:
--format <json|list> Changes the format of outputted workload versions. Can take 'json' or 'list'
| { | ||
| var featureBand = new SdkFeatureBand(SdkVersion); | ||
| var packageId = Installer.GetManifestPackageId(WorkloadPackageIdBase, featureBand); | ||
| var versions = (await PackageDownloader.GetLatestPackageVersions(packageId, NumberOfWorkloadSetsToTake, packageSourceLocation: null, includePreview: !string.IsNullOrWhiteSpace(SdkVersion.Prerelease)).ConfigureAwait(false)) |
There was a problem hiding this comment.
I haven't tested it, but this is my only real concern with this change:
If the user starts typing in a version, I don't see any way to adjust to autocomplete a version that starts with what they provided. Perhaps that isn't too important, but the current version in NuGetPackageDownloader doesn't even take feature band into account, so if a 9.0.403 user tries to autocomplete a workload set, but 10.0.1xx workload sets are out, they'd get that instead, and they can't just type 9 to fix things...
Ok, I started playing around a bit and got tab-completion working for the
--versionoption in the install and update commands.The core problem here was I needed to extract out the 'get workload set versions' logic you'd created into something that could be called from the tab-completion context. I also saw that you'd coded the functionality as an option on the workload search command - I disagree with this, the subcommand for listing versions should centralize this logic. So I pulled that out into a separate command, and then put the shared completions generation logic into a static member on this type that captures the logic involved. Other notes inline!
To test this, build the app and run the dogfood script, then run
dotnet complete "dotnet workload install --version "and you should get a list of versions out on stdout.