|
| 1 | +# PSDependScripts — Reviewer Checklist |
| 2 | + |
| 3 | +A guide for writing and reviewing dependency scripts in `PSDepend/PSDependScripts/`. |
| 4 | +Each script implements a single `DependencyType` (e.g. `PSGalleryModule`, `Git`, |
| 5 | +`Chocolatey`) and is dot-sourced by `Invoke-DependencyScript`. |
| 6 | + |
| 7 | +## Established patterns (best practices) |
| 8 | + |
| 9 | +### 1. Standard parameter contract |
| 10 | + |
| 11 | +- First parameter is `[PSTypeName('PSDepend.Dependency')][psobject[]]$Dependency`. |
| 12 | +- `$PSDependAction` is `[ValidateSet(...)][string[]]` with **only** the actions |
| 13 | + the script actually implements (`Test`, `Install`, `Import`) and defaults to |
| 14 | + `@('Install')`. |
| 15 | +- Type-specific options (`Repository`, `Source`, `Force`, `Global`, |
| 16 | + `ProviderName`, etc.) are top-level parameters — populated via |
| 17 | + `Parameters = @{ ... }` splat from the dependency hashtable. |
| 18 | + |
| 19 | +### 2. Comment-based help is mandatory and follows a shape |
| 20 | + |
| 21 | +- `.SYNOPSIS` / `.DESCRIPTION`. |
| 22 | +- A **"Relevant Dependency metadata"** block describing how each |
| 23 | + `$Dependency.*` field (`Name`, `Version`, `Target`, `Source`, `Credential`, |
| 24 | + `AddToPath`, `Parameters.*`) is interpreted *by this dependency type*. |
| 25 | +- A `.PARAMETER PSDependAction` block listing supported actions. |
| 26 | +- At least one `.EXAMPLE` showing a real `@{ }` dependency hashtable. Most |
| 27 | + scripts include both a simple and an advanced example. |
| 28 | + |
| 29 | +### 3. Field extraction at the top |
| 30 | + |
| 31 | +Resolve the inputs once into locals before the main body runs: |
| 32 | + |
| 33 | +- `$Name = $Dependency.Name`; fall back to `$Dependency.DependencyName`. |
| 34 | +- `$Version = $Dependency.Version`; default to `'latest'`. |
| 35 | +- `$Target` / `$Source` / `$Credential` with sensible defaults. |
| 36 | + |
| 37 | +### 4. `PSDependAction` semantics |
| 38 | + |
| 39 | +- `Test` alone returns `$true` / `$false`. |
| 40 | +- `Test` combined with `Install` returns `$true` early when satisfied and falls |
| 41 | + through to install otherwise. |
| 42 | +- `Install` does the work; `Import` (where supported) calls |
| 43 | + `Import-PSDependModule` against the resolved path. |
| 44 | +- The canonical "nothing found, test-only" guard appears verbatim across scripts: |
| 45 | + |
| 46 | + ```powershell |
| 47 | + if ($PSDependAction -contains 'Test' -and $PSDependAction.Count -eq 1) { |
| 48 | + return $false |
| 49 | + } |
| 50 | + ``` |
| 51 | + |
| 52 | +### 5. External tool prerequisites |
| 53 | + |
| 54 | +- Probe with `Get-Command <tool> -ErrorAction SilentlyContinue` and use |
| 55 | + `Write-Error` (not `throw`) when missing, so the dependency engine can |
| 56 | + continue with the rest of the run. |
| 57 | +- Invoke external tools via `Invoke-ExternalCommand` (see `Git.ps1`, |
| 58 | + `Chocolatey.ps1`) rather than `& tool`, so output capture is consistent. |
| 59 | + |
| 60 | +### 6. Path / scope semantics |
| 61 | + |
| 62 | +- `Target` doubles as Scope: `AllUsers` / `CurrentUser` are install scopes; |
| 63 | + any other value is a filesystem path (Save vs Install branch). |
| 64 | +- `AddToPath` consistently prepends to `$env:PATH` and/or `$env:PSModulePath` |
| 65 | + via `Add-ToItemCollection`. |
| 66 | + |
| 67 | +### 7. Logging discipline |
| 68 | + |
| 69 | +- `Write-Verbose` for normal progress on each decision branch. |
| 70 | +- `Write-Error` (not `throw`) for recoverable failures. |
| 71 | +- `Write-Warning` for skip-and-continue cases (see `Task.ps1`). |
| 72 | + |
| 73 | +## Reviewer checklist |
| 74 | + |
| 75 | +Use this as a PR review checklist when adding or modifying a script under |
| 76 | +`PSDepend/PSDependScripts/`. |
| 77 | + |
| 78 | +### Contract |
| 79 | + |
| 80 | +- [ ] First param is `[PSTypeName('PSDepend.Dependency')][psobject[]]$Dependency`. |
| 81 | +- [ ] `PSDependAction` is `[ValidateSet(...)]` and lists only implemented actions. |
| 82 | +- [ ] Type-specific params are top-level (not buried in |
| 83 | + `$Dependency.Parameters` lookups inside the body). |
| 84 | + |
| 85 | +### Help |
| 86 | + |
| 87 | +- [ ] `.SYNOPSIS` and `.DESCRIPTION` are present. |
| 88 | +- [ ] "Relevant Dependency metadata" block enumerates **every** `$Dependency.*` |
| 89 | + field the script reads. |
| 90 | +- [ ] Every parameter has a `.PARAMETER` entry. |
| 91 | +- [ ] At least one `.EXAMPLE` with a runnable `@{ }` hashtable. |
| 92 | + |
| 93 | +### Dependency-field handling |
| 94 | + |
| 95 | +- [ ] `Name` falls back to `DependencyName`. |
| 96 | +- [ ] `Version` defaults to `'latest'` (or documents why not). |
| 97 | +- [ ] `Target` default + scope-vs-path interpretation is documented. |
| 98 | +- [ ] `Credential` is honored when the underlying provider supports it. |
| 99 | +- [ ] `AddToPath` is honored where the install location is filesystem-based. |
| 100 | + |
| 101 | +### Action semantics |
| 102 | + |
| 103 | +- [ ] `Test` alone returns a single boolean. |
| 104 | +- [ ] `Test` + `Install` short-circuits cleanly when satisfied (no install, |
| 105 | + but `Import-PSDependModule` still runs if applicable). |
| 106 | +- [ ] Test-only "nothing found" returns `$false` via the canonical guard. |
| 107 | +- [ ] `Import` (if supported) goes through `Import-PSDependModule`. |
| 108 | + |
| 109 | +### Robustness |
| 110 | + |
| 111 | +- [ ] External tool dependencies probed via |
| 112 | + `Get-Command -ErrorAction SilentlyContinue`. |
| 113 | +- [ ] External invocations go through `Invoke-ExternalCommand`. |
| 114 | +- [ ] Failures use `Write-Error` (not `throw`) unless terminating is intended |
| 115 | + (e.g. `FailOnError`). |
| 116 | +- [ ] No `Out-Null` / `2>$null` swallowing of error streams. |
| 117 | +- [ ] Verbose messages on each decision branch. |
| 118 | +- [ ] Cross-platform paths use `Join-Path` (not string concat with `\`). |
| 119 | + |
| 120 | +### Version comparison (for installers) |
| 121 | + |
| 122 | +- [ ] Both `[SemanticVersion]::TryParse` and `[Version]::TryParse` are |
| 123 | + attempted before comparing (see `PSGalleryModule.ps1` lines 262–273). |
| 124 | +- [ ] `'latest'` vs explicit-version paths each produce a defensible |
| 125 | + early-return. |
| 126 | + |
| 127 | +### Security / hygiene |
| 128 | + |
| 129 | +- [ ] No plaintext credentials emitted in `Write-Verbose`. |
| 130 | +- [ ] No `Invoke-Expression` on dependency data. `Command.ps1` uses |
| 131 | + `[ScriptBlock]::Create` — that's the documented trust boundary; |
| 132 | + any new script doing this needs an explicit opt-in |
| 133 | + (e.g. `FailOnError`-style). |
| 134 | +- [ ] TLS 1.2 forced before `Invoke-WebRequest` against public registries |
| 135 | + (see `Chocolatey.ps1:182`). |
| 136 | + |
| 137 | +### Known smells to call out |
| 138 | + |
| 139 | +- Helper functions defined inside dependency scripts (e.g. `Parse-URLForFile` |
| 140 | + in `FileDownload.ps1`, `Get-ChocoInstalledPackage` in `Chocolatey.ps1`) |
| 141 | + should arguably live in `PSDepend/Private/`. Flag if a new script adds |
| 142 | + local helpers — both for reuse and because **inline helpers cannot be |
| 143 | + mocked by Pester**. |
| 144 | +- Direct `Invoke-Expression`-style dot-sourcing of user-supplied strings |
| 145 | + (see `Command.ps1`) requires explicit acknowledgement. |
0 commit comments