Skip to content

Conversation

@phenning
Copy link
Member

No description provided.

@phenning phenning requested a review from a team as a code owner October 29, 2025 12:20
return null;
}

string architecture = _environment.ProcessArchitecture.ToString().ToLowerInvariant();
Copy link
Member

Choose a reason for hiding this comment

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

Consider reusing:

private static string GetArchitectureSubKey(Architecture architecture)
{
return architecture switch
{
Architecture.X86 => "x86",
Architecture.X64 => "x64",
Architecture.Arm => "arm",
Architecture.Arm64 => "arm64",
_ => architecture.ToString().ToLower()
};

Avoids an allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

refactored out to an extensions method and renamed

}

private RetargetSDKDescription(string sdkVersion) : base(
private RetargetSDKDescription(string sdkVersion, string arch) : base(
Copy link
Member

Choose a reason for hiding this comment

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

Consider taking the Architecture enum here so that it's clear what domain of values are valid. Then do the conversion to lower case whatever internally when formatting a string.

* DotNetEnvironment.GetArchitectureSubKey(Architecture) into an Architecture extension method.
* Updated old call sites
* Updated this PR to pass the Architecture to RetargetSDKDescription and do the string replacement there.
* Updated unit test to verify expected architecture is present in the suggesion Url.
@phenning phenning merged commit cbad870 into main Oct 30, 2025
5 checks passed
@phenning phenning deleted the phenning/archLink branch October 30, 2025 01:50
@dotnet-policy-service dotnet-policy-service bot added this to the 18.1 milestone Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants