Add SDK version utilities and GlobalJson parsing#2550
Add SDK version utilities and GlobalJson parsing#2550phenning wants to merge 1 commit intodotnet:mainfrom
Conversation
Add new utilities for SDK version parsing and comparison: - parseSdkVersion: Parse SDK version string into components - isCompatibleSdkVersion: Check version compatibility with rollForward policy - getCompatibleSdkVersions: Filter versions by compatibility - compareSdkVersions: Compare two SDK versions - isNewerSdkVersion: Check if one version is newer than another - isValidNumber: Exported helper for number validation Add GlobalJson parsing utilities: - GlobalJson, GlobalJsonSdk, GlobalJsonRequirements interfaces - parseGlobalJsonContent: Parse global.json content - getRequirementsFromGlobalJson: Extract SDK requirements - parseGlobalJsonRequirements: Combined parse and extract - getEffectiveRollForward: Get effective roll-forward policy - getEffectiveAllowPrerelease: Get effective prerelease setting Register VS Code commands for external extension consumption: - dotnet.parseSdkVersion - dotnet.isCompatibleSdkVersion - dotnet.getCompatibleSdkVersions - dotnet.compareSdkVersions - dotnet.isNewerSdkVersion - dotnet.parseGlobalJson Add unit tests for all new SDK version utilities.
| return requirements.rollForward; | ||
| } | ||
| // Default is 'latestPatch' when version is specified, 'latestMajor' otherwise | ||
| return requirements.sdkVersion ? 'latestPatch' : 'latestMajor'; |
There was a problem hiding this comment.
Should we support more than latestPatch and latestMajor?
| return requirements.rollForward; | ||
| } | ||
| // Default is 'latestPatch' when version is specified, 'latestMajor' otherwise | ||
| return requirements.sdkVersion ? 'latestPatch' : 'latestMajor'; |
There was a problem hiding this comment.
I'm wondering if we should store these as static constant strings instead of having them coded here.
| test('parseSdkVersion parses SDK versions correctly', async () => | ||
| { | ||
| const v1 = resolver.parseSdkVersion('8.0.308'); | ||
| assert.equal(v1.major, 8); |
There was a problem hiding this comment.
I like the conciseness of the tests here!
| * @param rollForward The rollForward policy (defaults to 'latestPatch' if not specified) | ||
| * @returns true if the installed SDK satisfies the requirement | ||
| */ | ||
| export function isCompatibleSdkVersion(installedVersion: string, requiredVersion: string, rollForward: RollForwardPolicy = 'latestPatch'): boolean |
There was a problem hiding this comment.
stringVersionMeetsRequirement already exists as a method to compare strings against rollforward values. Perhaps it should be moved here and utilized as such.
| * The rollForward policy from global.json that determines SDK version selection behavior. | ||
| * Reference: https://learn.microsoft.com/en-us/dotnet/core/tools/global-json | ||
| */ | ||
| export type RollForwardPolicy = 'disable' | 'patch' | 'latestPatch' | 'feature' | 'latestFeature' | 'minor' | 'latestMinor' | 'major' | 'latestMajor'; |
There was a problem hiding this comment.
I like the docstring here and the name makes more sense to me, but these exist to some degree already in simplifiedVersionSpec - we should consolidate them.
| * @param version The SDK version string | ||
| * @returns Parsed version components | ||
| */ | ||
| export function parseSdkVersion(version: string): ParsedSdkVersion |
There was a problem hiding this comment.
I think this is a clean approach to have a class like this but it is not the most performant approach since it parses many parts multiple times. The logic that compares versions already in the code short-circuits the evaluations of the string and prevents duplicated splits.
| * Gets the full patch field from an SDK version string (e.g., 308 from "8.0.308"). | ||
| * @returns The full patch number, or undefined if it cannot be extracted | ||
| */ | ||
| function getFullPatchFromVersionSimple(version: string): number | undefined |
There was a problem hiding this comment.
This seems similar to getSDKFeatureBandOrPatchFromFullySpecifiedVersion (albeit a less verbose name) - I'm thinking many of these functions are not necessary.
| open(url).catch(() => {}); | ||
| }); | ||
|
|
||
| const compareSdkVersionsRegistration = vscode.commands.registerCommand(`${commandPrefix}.${commandKeys.compareSdkVersions}`, |
There was a problem hiding this comment.
The most important feedback I have here:
I think it's wise for this logic to live here, and we shouldn't reimplement what's done in recommendedVersion and listVersions. Although I think the overhead of the vscode command calls is quite high. The fact that the codebase parses versions as strings instead of a type is definitely a regret and hard to change from the initial design, which is why we haven't replaced it with something like semver. For new code, this makes me think the version parsing/compare could live in CDK instead if there was a smarter type around it, for the sake of not repeating a bad pattern.
If we continue with this, really the best approach would be for some of the version utilities ( and our types) to be put on npm as a library dependency, considering there's no shared caching/persistent data. I have avoided doing that as there's never been a big drive to do so - (although it hurts seeing others have to copy our type files instead of using @types/.)
I understand doing so would:
A - create a new work item
B - create a new maintenance point (which we could hopefully automate the release of alongside new extension releases)
C - delay the progress of your feature
I'm curious on your opinion w.r.t this - it may be best to have it live in CDK for now.
|
I think the new PR update plans on having this code live in CDK for now - is this correct? 👀 |
I think @JakeRadMSFT would prefer that it didn’t live in CDK, but we can hold this PR (and fix it) so that we can make that call later? |
2 participants
Add new utilities for SDK version parsing and comparison:
Add GlobalJson parsing utilities:
Register VS Code commands for external extension consumption:
Add unit tests for all new SDK version utilities.