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

Crazy workload version tab-completion idea #2

Open
wants to merge 1 commit into
base: option-list-workload-sets
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/Cli/dotnet/commands/InstallingWorkloadCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ protected void UpdateWorkloadManifests(WorkloadHistoryRecorder recorder, ITransa
}
}

string resolvedWorkloadSetVersion = _workloadSetVersionFromGlobalJson ??_workloadSetVersionFromCommandLine;
string resolvedWorkloadSetVersion = _workloadSetVersionFromGlobalJson ?? _workloadSetVersionFromCommandLine;
if (string.IsNullOrWhiteSpace(resolvedWorkloadSetVersion) && !UseRollback && !FromHistory)
{
_workloadManifestUpdater.UpdateAdvertisingManifestsAsync(_includePreviews, updateToLatestWorkloadSet, offlineCache).Wait();
Expand Down Expand Up @@ -423,10 +423,14 @@ protected IEnumerable<WorkloadId> WriteSDKInstallRecordsForVSWorkloads(IEnumerab

internal static class InstallingWorkloadCommandParser
{
public static readonly CliOption<string> WorkloadSetVersionOption = new("--version")
public static readonly CliOption<string> WorkloadSetVersionOption = new CliOption<string>("--version")
{
Description = Strings.WorkloadSetVersionOptionDescription
};
}.AddCompletions((ctx) =>
{
var versions = Search.Versions.SearchWorkloadSetsCommand.GetWorkloadSetVersions(ctx).GetAwaiter().GetResult();
return versions.Select(v => new System.CommandLine.Completions.CompletionItem(v));
});
Comment on lines +429 to +433
Copy link
Author

Choose a reason for hiding this comment

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

Here's the completions hookup - we needed something that could take the completion-time parse result and turn it into CompletionItems.


public static readonly CliOption<bool> PrintDownloadLinkOnlyOption = new("--print-download-link-only")
{
Expand Down
4 changes: 2 additions & 2 deletions src/Cli/dotnet/commands/dotnet-complete/CompleteCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Author

Choose a reason for hiding this comment

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

tiny change for making debugging easier!

return completions.Distinct()
.ToArray();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,17 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.CommandLine;
using System.Text.Json;
Copy link
Author

Choose a reason for hiding this comment

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

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.

using Microsoft.Deployment.DotNet.Releases;
using Microsoft.DotNet.Cli;
using Microsoft.DotNet.Cli.NuGetPackageDownloader;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.Workloads.Workload.Install;
using Microsoft.NET.Sdk.WorkloadManifestReader;
using Microsoft.TemplateEngine.Cli.Commands;

namespace Microsoft.DotNet.Workloads.Workload.Search
{
internal class WorkloadSearchCommand : WorkloadCommandBase
{
private readonly IWorkloadResolver _workloadResolver;
private readonly ReleaseVersion _sdkVersion;
private readonly string _workloadIdStub;
private readonly int _numberOfWorkloadSetsToTake;
private readonly string _workloadSetOutputFormat;
private readonly IWorkloadManifestInstaller _installer;
internal bool ListWorkloadSetVersions { get; set; } = false;

public WorkloadSearchCommand(
ParseResult result,
Expand All @@ -31,53 +22,17 @@ public WorkloadSearchCommand(
_workloadIdStub = result.GetValue(WorkloadSearchCommandParser.WorkloadIdStubArgument);

workloadResolverFactory = workloadResolverFactory ?? new WorkloadResolverFactory();
var creationResult = workloadResolverFactory.Create();
_workloadResolver = creationResult.WorkloadResolver;

if (!string.IsNullOrEmpty(result.GetValue(WorkloadSearchCommandParser.VersionOption)))
{
throw new GracefulException(Install.LocalizableStrings.SdkVersionOptionNotSupported);
}

var creationResult = workloadResolverFactory.Create();

_sdkVersion = creationResult.SdkVersion;
_workloadResolver = creationResult.WorkloadResolver;

_numberOfWorkloadSetsToTake = result.GetValue(SearchWorkloadSetsParser.TakeOption);
_workloadSetOutputFormat = result.GetValue(SearchWorkloadSetsParser.FormatOption);

_installer = WorkloadInstallerFactory.GetWorkloadInstaller(
reporter,
new SdkFeatureBand(_sdkVersion),
_workloadResolver,
Verbosity,
creationResult.UserProfileDir,
!SignCheck.IsDotNetSigned(),
restoreActionConfig: new RestoreActionConfig(result.HasOption(SharedOptions.InteractiveOption)),
elevationRequired: false,
shouldLog: false);
}

public override int Execute()
{
if (ListWorkloadSetVersions)
{
var featureBand = new SdkFeatureBand(_sdkVersion);
var packageId = _installer.GetManifestPackageId(new ManifestId("Microsoft.NET.Workloads"), featureBand);
var versions = PackageDownloader.GetLatestPackageVersions(packageId, _numberOfWorkloadSetsToTake, packageSourceLocation: null, includePreview: !string.IsNullOrWhiteSpace(_sdkVersion.Prerelease))
.GetAwaiter().GetResult()
.Select(version => WorkloadManifestUpdater.WorkloadSetPackageVersionToWorkloadSetVersion(featureBand, version.Version.ToString()));
if (_workloadSetOutputFormat?.Equals("json", StringComparison.OrdinalIgnoreCase) == true)
{
Reporter.WriteLine(JsonSerializer.Serialize(versions.Select(version => version.ToDictionary(_ => "workloadVersion", v => v))));
}
else
{
Reporter.WriteLine(string.Join('\n', versions));
}

return 0;
}

IEnumerable<WorkloadResolver.WorkloadInfo> availableWorkloads = _workloadResolver.GetAvailableWorkloads()
.OrderBy(workload => workload.Id);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.CommandLine;
using System.CommandLine.Completions;
using System.Text.Json;
using Microsoft.Deployment.DotNet.Releases;
using Microsoft.DotNet.Cli;
using Microsoft.DotNet.Cli.NuGetPackageDownloader;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.DotNet.Workloads.Workload.Install;
using Microsoft.NET.Sdk.WorkloadManifestReader;
using Microsoft.TemplateEngine.Cli.Commands;

namespace Microsoft.DotNet.Workloads.Workload.Search.Versions;

internal class SearchWorkloadSetsCommand : WorkloadCommandBase
{
internal readonly int NumberOfWorkloadSetsToTake;
private readonly SearchWorkloadSetsFormat _workloadSetOutputFormat;
internal readonly IWorkloadManifestInstaller Installer;
internal readonly ReleaseVersion SdkVersion;
private readonly IWorkloadResolver _workloadResolver;

private static readonly ManifestId WorkloadPackageIdBase = new("Microsoft.NET.Workloads");

public SearchWorkloadSetsCommand(ParseResult result, IWorkloadResolverFactory workloadResolverFactory = null, IReporter reporter = null) : base(result, reporter: reporter)
{
NumberOfWorkloadSetsToTake = result.HasOption(SearchWorkloadSetsParser.TakeOption) ? result.GetValue(SearchWorkloadSetsParser.TakeOption) : 5;
_workloadSetOutputFormat = result.GetValue(SearchWorkloadSetsParser.FormatOption);
workloadResolverFactory = workloadResolverFactory ?? new WorkloadResolverFactory();
var creationResult = workloadResolverFactory.Create();

SdkVersion = creationResult.SdkVersion;
_workloadResolver = creationResult.WorkloadResolver;

Installer = WorkloadInstallerFactory.GetWorkloadInstaller(
reporter,
new SdkFeatureBand(SdkVersion),
_workloadResolver,
Verbosity,
creationResult.UserProfileDir,
!SignCheck.IsDotNetSigned(),
restoreActionConfig: new RestoreActionConfig(result.HasOption(SharedOptions.InteractiveOption)),
elevationRequired: false,
shouldLog: false);
}

protected override void ShowHelpOrErrorIfAppropriate(ParseResult parseResult)
{

}

Comment on lines +49 to +53
Copy link
Author

Choose a reason for hiding this comment

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

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))
Copy link
Owner

Choose a reason for hiding this comment

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

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...

.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;
}
Comment on lines +54 to +70
Copy link
Author

Choose a reason for hiding this comment

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

This is all the same logic from the other class, just in this new class.


public override int Execute() => Execute(CancellationToken.None).GetAwaiter().GetResult();

/// <summary>
/// Used for CLI completions to get the workload set versions.
/// </summary>
/// <returns></returns>
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;
}
Comment on lines +78 to +86
Copy link
Author

Choose a reason for hiding this comment

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

This is a simple static accessor to the same core logic in Execute, for tab completion purposes.

Copy link
Owner

Choose a reason for hiding this comment

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

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.CommandLine;
using Microsoft.DotNet.Workloads.Workload.Search;
using Microsoft.DotNet.Workloads.Workload.Search.Versions;
using LocalizableStrings = Microsoft.DotNet.Workloads.Workload.Search.LocalizableStrings;

namespace Microsoft.DotNet.Cli
Expand All @@ -11,7 +11,7 @@ internal static class SearchWorkloadSetsParser
{
public static readonly CliOption<int> TakeOption = new("--take") { DefaultValueFactory = (_) => 5 };

public static readonly CliOption<string> FormatOption = new("--format")
public static readonly CliOption<SearchWorkloadSetsFormat> FormatOption = new("--format")
{
Description = LocalizableStrings.FormatOptionDescription
};
Expand All @@ -37,12 +37,15 @@ private static CliCommand ConstructCommand()
}
});

command.SetAction(parseResult => new WorkloadSearchCommand(parseResult)
{
ListWorkloadSetVersions = true
}.Execute());
command.SetAction((parseResult, ct) => new SearchWorkloadSetsCommand(parseResult).Execute(ct));

return command;
}
}

internal enum SearchWorkloadSetsFormat
{
list,
json
}
Comment on lines +46 to +50
Copy link
Author

@baronfel baronfel Aug 29, 2024

Choose a reason for hiding this comment

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

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'

}