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

Better search versions error experience #45144

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Nov 26, 2024

If you try dotnet workload search version and then put anything after that that it doesn't recognize, it assumes it's a workload version argument. Often, it isn't. For instance, dotnet workload search version --include-previews tries to parse --include-previews as a version incorrectly, fails, and throws a garbage error. This makes the error experience more useful.

For 'workloadVersionArgument's that aren't really workload version arguments
@Forgind Forgind requested review from a team, tmat, AntonLapounov and vijayrkn as code owners November 26, 2024 14:01
@Forgind Forgind changed the base branch from main to release/9.0.2xx November 26, 2024 14:01
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels Nov 26, 2024
Copy link
Contributor

Thanks for your PR, @Forgind.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@Forgind Forgind removed request for a team, tmat, vijayrkn and AntonLapounov November 26, 2024 14:01
@KalleOlaviNiemitalo
Copy link
Contributor

This workaround would not be needed if dotnet/command-line-api#1860 were fixed.

@marcpopMSFT marcpopMSFT added Area-Workloads and removed Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch labels Nov 26, 2024
public static bool IsWorkloadSetPackageVersion(string workloadSetVersion)
{

string[] sections = workloadSetVersion.Split(['-', '+'], 2);
Copy link
Member Author

Choose a reason for hiding this comment

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

The first few lines here are duplicated from below, but they can't really be de-duped, as they are just defining variables. The variables would need to be defined either way.

Comment on lines 60 to 66
if (versionArgument is not null)
{
if (!WorkloadSetVersion.IsWorkloadSetPackageVersion(versionArgument))
{
result.AddError(string.Format(CommandLineValidation.LocalizableStrings.UnrecognizedCommandOrArgument, versionArgument));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: Combine nested boolean conditions

Suggested change
if (versionArgument is not null)
{
if (!WorkloadSetVersion.IsWorkloadSetPackageVersion(versionArgument))
{
result.AddError(string.Format(CommandLineValidation.LocalizableStrings.UnrecognizedCommandOrArgument, versionArgument));
}
}
if (versionArgument is not null &&
!WorkloadSetVersion.IsWorkloadSetPackageVersion(versionArgument))
{
result.AddError(string.Format(CommandLineValidation.LocalizableStrings.UnrecognizedCommandOrArgument, versionArgument));
}

Comment on lines 11 to 27
public static bool IsWorkloadSetPackageVersion(string workloadSetVersion)
{

string[] sections = workloadSetVersion.Split(['-', '+'], 2);
string versionCore = sections[0];
string? preReleaseOrBuild = sections.Length > 1 ? sections[1] : null;

string[] coreComponents = versionCore.Split('.');
return coreComponents.Length >= 3 && coreComponents.Length <= 4;
}

public static string ToWorkloadSetPackageVersion(string workloadSetVersion, out SdkFeatureBand sdkFeatureBand)
{
string[] sections = workloadSetVersion.Split(new char[] { '-', '+' }, 2);
string[] sections = workloadSetVersion.Split(['-', '+'], 2);
string versionCore = sections[0];
string? preReleaseOrBuild = sections.Length > 1 ? sections[1] : null;

Copy link
Member

@MiYanni MiYanni Dec 3, 2024

Choose a reason for hiding this comment

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

Deduplicated the code. Also, noticed preReleaseOrBuild is unused in IsWorkloadSetPackageVersion. I cannot do a suggestion block since I cannot highlight the bottom of the ToWorkloadSetPackageVersion method in GitHub to include in this comment.

        private static string[] SeparateCoreComponents(string workloadSetVersion, out string[] sections)
        {
            sections = workloadSetVersion.Split(['-', '+'], 2);
            return sections[0].Split('.');
        }

        public static bool IsWorkloadSetPackageVersion(string workloadSetVersion)
        {
            string[] coreComponents = SeparateCoreComponents(workloadSetVersion, out _);
            return coreComponents.Length >= 3 && coreComponents.Length <= 4;
        }

        public static string ToWorkloadSetPackageVersion(string workloadSetVersion, out SdkFeatureBand sdkFeatureBand)
        {
            string[] coreComponents = SeparateCoreComponents(workloadSetVersion, out string[] sections);
            string major = coreComponents[0];
            string minor = coreComponents[1];
            string patch = coreComponents[2];
            string packageVersion = $"{major}.{patch}.";
            if (coreComponents.Length == 3)
            {
                //  No workload set patch version
                packageVersion += "0";
                //  Use preview specifier (if any) from workload set version as part of SDK feature band
                sdkFeatureBand = new SdkFeatureBand(workloadSetVersion);
            }
            else
            {
                //  Workload set version has workload patch version (ie 4 components)
                packageVersion += coreComponents[3];
                //  Don't include any preview specifiers in SDK feature band
                sdkFeatureBand = new SdkFeatureBand($"{major}.{minor}.{patch}");
            }

            if (sections.Length > 1)
            {
                //  Figure out if we split on a '-' or '+'
                char separator = workloadSetVersion[sections[0].Length];
                packageVersion += separator + sections[1];
            }
            return packageVersion;
        }

@marcpopMSFT marcpopMSFT merged commit 152a5d2 into dotnet:release/9.0.2xx Dec 10, 2024
28 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Workloads untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants