fix: check all local versions before remote when explicit version requested#176
Open
HeyItsGilbert wants to merge 3 commits into
Open
fix: check all local versions before remote when explicit version requested#176HeyItsGilbert wants to merge 3 commits into
HeyItsGilbert wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves dependency resolution performance and correctness when an explicit version is requested, by preferring locally installed matches (across all installed versions) before making any remote registry calls, and by making version comparisons more robust than raw string equality.
Changes:
PSGalleryModule.ps1/Package.ps1: check all installed versions for an explicit requested version before falling through to remote lookup/install logic.PSGalleryModule.ps1/Package.ps1/Nuget.ps1/PSGalleryNuget.ps1: replace raw string equality with multi-step typed version comparison (Version → SemanticVersion → fallback).docs/PSDependScripts-ReviewerChecklist.md: document “local-first resolution” as a standard script pattern/checklist item.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| PSDepend/PSDependScripts/PSGalleryNuget.ps1 | Adds typed version comparison for explicit-version early return to avoid unnecessary remote calls. |
| PSDepend/PSDependScripts/PSGalleryModule.ps1 | Adds explicit-version local-first matching across all installed module versions before remote lookup. |
| PSDepend/PSDependScripts/Package.ps1 | Adds explicit-version local-first matching across all installed package versions before remote lookup. |
| PSDepend/PSDependScripts/Nuget.ps1 | Adds typed version comparison for explicit-version early return to avoid unnecessary remote calls. |
| docs/PSDependScripts-ReviewerChecklist.md | Adds checklist guidance documenting the local-first explicit-version resolution pattern. |
Comments suppressed due to low confidence (4)
PSDepend/PSDependScripts/PSGalleryNuget.ps1:146
- The
[System.Version]equality path here is still strict:Version.Parse('1.0')is not equal toVersion.Parse('1.0.0.0')(missing components become-1). Since[SemanticVersion]::TryParse()also rejects 4-part versions, the current logic will still treat"1.0"vs"1.0.0.0"as different and fall through unnecessarily. Consider normalizing both versions (treat missing build/revision as0) before comparing, so the behavior matches the docstring/PR intent.
$versionMatches = if (
[System.Version]::TryParse($Version, [ref]$parsedRequestedVersion) -and
[System.Version]::TryParse($ExistingVersion, [ref]$parsedExistingVersionCheck)
) {
$parsedRequestedVersion -eq $parsedExistingVersionCheck
PSDepend/PSDependScripts/PSGalleryModule.ps1:234
- This local-match check uses strict
[version]equality ($_.Version -eq $parsedRequestedVersion), which does not treat"1.0"and"1.0.0.0"as equal. If a module manifest uses 2- or 3-part versions but the installed module reports 4-part (or vice versa), the match will fail and trigger a remote call contrary to the intent. Consider normalizing theSystem.Versionvalues (pad missing build/revision with 0) before comparing.
$matchedInstall = if ([System.Version]::TryParse($Version, [ref]$parsedRequestedVersion)) {
$Existing | Where-Object { $_.Version -eq $parsedRequestedVersion } | Select-Object -First 1
} elseif ([System.Management.Automation.SemanticVersion]::TryParse($Version, [ref]$parsedRequestedSemanticVersion)) {
PSDepend/PSDependScripts/Package.ps1:189
- The string-fallback branch isn’t actually doing raw string equality:
$_ .Version -eq $Versionwill coerce$Versionto[version]when$_.Versionis aVersion, reintroducing the same formatting mismatch this change is trying to avoid. Use explicit string comparison (e.g., compare.ToString()results) for the fallback path.
} else {
$Existing | Where-Object { $_.Version -eq $Version } | Select-Object -First 1
}
PSDepend/PSDependScripts/Nuget.ps1:163
- The
[System.Version]equality path is still strict:Version.Parse('1.0')is not equal toVersion.Parse('1.0.0.0')(missing components become-1). Since[SemanticVersion]::TryParse()also rejects 4-part versions, the current logic will still treat"1.0"vs"1.0.0.0"as different and fall through unnecessarily. Normalize/pad version components (treat missing build/revision as0) before comparing so explicit-version requests behave as intended.
[System.Version]::TryParse($Version, [ref]$parsedRequestedVersion) -and
[System.Version]::TryParse($ExistingVersion, [ref]$parsedExistingVersionCheck)
) {
$parsedRequestedVersion -eq $parsedExistingVersionCheck
} elseif (
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uested When a Dependency declares an explicit version, all locally installed versions are now checked before any remote registry call is made. Previously PSGalleryModule and Package only compared against the maximum installed version, so requesting version 1.0 when 1.0 and 2.0 were both installed would fall through to Find-Module/Find-Package unnecessarily. All four scripts also used raw string equality, causing mismatches between "1.0" and "1.0.0.0". - PSGalleryModule.ps1 / Package.ps1: iterate all installed versions using [System.Version] -> [SemanticVersion] -> string fallback before hitting remote - Nuget.ps1 / PSGalleryNuget.ps1: replace string equality with same three-step version comparison - docs/PSDependScripts-ReviewerChecklist.md: document local-first resolution as an established pattern and reviewer checklist item Fixes #97 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…function
The [System.Version] / [SemanticVersion] / string-fallback comparison
pattern was repeated across four DependencyScripts. Extract it into
PSDepend/Private/Test-VersionEquality.ps1 so the logic lives in one place
and each script reduces to a single Where-Object predicate or an if call.
Also fixes [System.Version] equality for 2-part vs 4-part strings
("1.0" vs "1.0.0.0") by normalising Build/Revision from -1 to 0
before comparing — addresses Copilot review feedback on PR #176.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9031a27 to
4c2cf9b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PSGalleryModuleandPackagepreviously only compared against the maximum installed version — requesting1.0when both1.0and2.0were installed would fall through toFind-Module/Find-Packageunnecessarily"1.0"and"1.0.0.0"Scripts updated:
PSGalleryModule.ps1/Package.ps1: iterate all installed versions using[System.Version]→[SemanticVersion]→ string fallback before hitting remoteNuget.ps1/PSGalleryNuget.ps1: replace string equality with the same three-step version comparisondocs/PSDependScripts-ReviewerChecklist.md: document local-first resolution as an established pattern and checklist itemFixes #97
Test plan
.\build.ps1 StageFilesto stage changesPSGalleryModule.Type.Tests.ps1(11 tests) — explicit version and latest paths both coveredNuget.Type.Tests.ps1(6 tests) — version comparison paths coveredPSGalleryNuget.Type.Tests.ps1(6 tests) — version comparison paths coveredPackage.Type.Tests.ps1(4 tests) — existing version paths covered🤖 Generated with Claude Code