# Contributing Thank you for considering a contribution. This tool deletes files as an administrator on production print servers, so the bar for changes is deliberately high — and the process is designed so that a contribution can be validated mechanically rather than argued about. --- ## Before you start - **Read the [wiki](../../wiki)**, especially [Safety Model](../../wiki/Safety-Model) and [For Developers](../../wiki/For-Developers). - **Open an issue first** for anything beyond a typo or a documentation fix. A short description of the problem and your intended approach saves everyone time if the direction is wrong. - **Check the troubleshooting matrix** before reporting a bug: many "failures" are guards working correctly against a corrupt `DefaultSpoolDirectory`. --- ## The one rule that matters most **Edit `src/`, never `dist/`.** `dist/` is generated by `tools/Build.ps1` and is overwritten on every build. Both distributables — the standalone `.ps1` and the `PrintSpoolerCleanup` module — are assembled from the same sources, which is precisely so they cannot drift apart. v3 hand-maintained two copies and they disagreed with each other and with the README. --- ## Development setup ```powershell git clone https://github.com/paulmann/Print-Spooler-Queue-Cleanup.git cd Print-Spooler-Queue-Cleanup ./tools/Build.ps1 -SkipTests -NoZip # assemble dist/ ./tests/Run-Tests.ps1 # full quality gate ``` Requirements: PowerShell 7.x for development (the built artifacts still target 5.1). No modules — the test harness is dependency-free by design, so the suite runs on a clean machine and on a Linux CI runner. Fast inner loop: ```powershell ./tools/Build.ps1 -SkipTests -NoZip; ./tests/Run-Tests.ps1 -UnitOnly ``` --- ## The quality gate `tests/Run-Tests.ps1` runs six stages and exits non-zero if any fails. **Your PR must pass all six.** | Stage | What it proves | |---|---| | 1. Build | `src/` assembles into both distributables | | 2. Parse validation | Every source file *and* every generated artifact parses with zero errors | | 3. Source lint | 12 rules — the PowerShell traps this codebase has already been bitten by have not returned | | 4. Batch lint | Labels resolve, `shift` counts match, no duplicate switches, pure ASCII, CRLF, no own delete loop | | 5. Unit tests | 205 assertions against the **built artifact**, not the sources | | 6. Module import | 36 assertions on the `.psd1`/`.psm1` form — a genuinely separate code path | | 7. Smoke test | 215 end-to-end assertions in a separate process | | 8. Release integrity | 46 assertions: checksums, ZIP safety, version coherence, docs, licence | | 9. Brief compliance | 123 assertions that every requirement from the original brief still works | 697 checks in total. Run the gate **both ways** before you push — `./tests/Run-Tests.ps1` directly and via `./tools/Build.ps1`. Three defects in this project reproduced only on the nested path, because that is the only one where a parent process had already exported environment variables and left a `$LASTEXITCODE` behind. Unit tests run against `dist/` on purpose: a build defect must not be able to hide behind a source-level pass. --- ## Testing expectations - **New behaviour needs a test.** Add it to `tests/Unit.*.ps1` (harness: `tests/TestHarness.ps1`). - **A bug fix needs a test that failed before the fix.** This is not bureaucracy — it is how the cooldown bug, the `switch` multi-match bug and the `GetFullPath` platform bug were pinned down. - **A guard change needs both an acceptance and a refusal test.** Guards are only meaningful if you can see what they reject *and* what they must still allow. Note that a legitimate relocated spool root such as `D:\Spool\PRINTERS` must be accepted: a guard that stops administrators doing their job will be disabled, and then it protects nothing. - **Cross-platform matters.** The Linux CI leg is not decoration — it caught a safety guard that silently passed on Windows because `[IO.Path]::GetFullPath()` is platform-relative. No Pester. The product promises "no external modules" and the test suite holds itself to the same rule. --- ## Style - **English** for code, comments, documentation and commit messages. - Comment-based help (`.SYNOPSIS` / `.DESCRIPTION` / `.PARAMETER` / `.EXAMPLE`) on every public function and every non-obvious private one. - Comments explain **why**, not what. The code says what. A comment that records a trap someone will otherwise rediscover is worth more than ten that restate the obvious. - `[CmdletBinding()]` and `[OutputType()]` on functions that return data. - `SupportsShouldProcess` on anything destructive. - No nested `function` declarations — they leak into script scope once the build inlines the file. Use a scriptblock closure with a **multi-character name** (PowerShell variables are case-insensitive, so `$W` and `$w` are the same variable). - No `Write-Host` in a function that also returns data, unless it is part of the rendering layer. - No empty `catch { }`. If a swallow is deliberate, say why inside the block. Both are lint-enforced. - No tabs. No trailing whitespace. --- ## PowerShell traps [For Developers](../../wiki/For-Developers#powershell-traps-that-already-bit-this-project) keeps a numbered list of fifteen traps this project has already hit — `return switch` being a syntax error, `$'` swallowing a string terminator, `switch` not stopping at the first match, `OrderedDictionary` having no `.ContainsKey()`, `Mandatory` rejecting empty strings before your guard runs, enumerating `.Keys` while assigning into the same hashtable, and more. **Read it before writing code, and add to it when you hit something new.** If a trap can recur, add a source-lint rule in `tests/Run-Tests.ps1` so it cannot. --- ## Adding a parameter Nine steps, listed in full in [For Developers](../../wiki/For-Developers#adding-a-parameter). Two are test-enforced: - a parameter without a description in `Get-ParameterDocumentation` **fails `-SelfTest`** - a description for a parameter that does not exist is dead code the unit suite flags Also update: `CLI-Reference.md`, the batch launcher if it should be reachable from `cmd`, `Get-DefaultConfiguration` + `Test-Configuration` if it deserves a site-wide default, and `CHANGELOG.md`. --- ## Adding a payload token 1. `@@YOUR_TOKEN@@` in the template (`src/Private/06-RemotePayload.ps1`) 2. A mapping in `New-PayloadTokenMap` (`src/Private/07-Orchestration.ps1`) 3. An entry in `$payloadTokenWhitelist` in `tools/Build.ps1` 4. An assertion in `tests/Unit.Guards.ps1` proving it substituted For an **array** value use `ConvertTo-QuotedListLiteral` and write `@(@@YOUR_TOKEN@@)` in the template — not `@('@@YOUR_TOKEN@@')`, which produces `@(''a', 'b'')` and fails to parse. Skipping step 3 fails the build with "unexpected tokens", which is the intended outcome. --- ## Commit messages Imperative mood, a summary line of ≤72 characters, then the *why*: ``` Refuse spool directories that are not under a 'spool' folder The forbidden-root guard compared for equality only, so a corrupt DefaultSpoolDirectory pointing at C:\Users\PRINTERS passed: right leaf, right depth, not equal to any protected root. A structural rule defeats that whole class of accident. ``` Reference an issue where one exists. Note any trap you hit, so it can go in the list. --- ## Pull request checklist - [ ] `./tests/Run-Tests.ps1` passes all six stages locally - [ ] New behaviour has tests; a bug fix has a test that failed before it - [ ] `src/` changed, not `dist/` - [ ] Comment-based help added or updated - [ ] `CHANGELOG.md` updated under an `[Unreleased]` or version heading - [ ] Wiki pages updated if user-visible behaviour changed (`docs/wiki/` mirrors the GitHub wiki) - [ ] README updated if a user-facing flag or exit code changed - [ ] No new external module dependency - [ ] If a guard changed: both a refusal test and an acceptance test - [ ] If you hit a PowerShell trap: it is recorded in For Developers, and lint-guarded if it can recur --- ## What gets rejected Honestly, so nobody wastes an evening: - **A second implementation of the delete step.** One destructive code path, run everywhere. A "fast batch mode" or a "lightweight local path" reintroduces exactly the divergence v3 suffered from. - **Weakening a safety guard** without an equally strong replacement. If a guard blocks a legitimate layout, the answer is a documented, deliberate override (`-SpoolDirectory`), not a looser guard. - **Recursive deletion** of the spool directory, under any circumstances. - **A wildcard in `Cleanup.IncludeExtensions`.** Rejected by validation, and it should stay that way. - **A new external module dependency.** The product's core promise is that it runs on a clean machine with nothing installed. - **Silent failure.** If a step can fail, it must be logged and reflected in the result or the exit code. An empty `catch { }` is lint-blocked. - **Generated files in the diff.** `dist/` is a build artifact. --- ## Releases Maintainers only — see the release checklist in [For Developers](../../wiki/For-Developers#release-checklist). The headline points: - bump `$Script:ScriptVersion` in `src/Shared/00-Constants.ps1` (the single source of truth; the build stamps it into the `.ps1`, `.psd1`, `.bat` and release notes) - update `CHANGELOG.md` — the build extracts the release body from it - upload **every** file in `dist/`, including `checksums.txt`. Clients refuse to install an update whose checksums are missing, so forgetting it breaks the update path for everyone. --- ## Conduct See [CODE_OF_CONDUCT.md](./CODE_OF_CONDUCT.md). --- ## Questions Open an issue, or email **Mikhail@Deynekin.com**. For a **security** issue, email rather than opening a public issue — see [SECURITY.md](./SECURITY.md).