pr-readiness

# PR Readiness ## PR Requirements Checklist ### 1. PR Title The title must clearly and concisely describe the change from the user's perspective. It should: - Start with a verb (e.g., "Fix", "Add", "Improve", "Remove", "Update"). - Mention the affected feature or area (e.g., presets, kits, CTest, build tasks, Project Outline). - Be specific enough that a reader understands the change without opening the PR. **Good examples:** - `Fix preset reloading loop when preset files are symlinks` - `Add "Delete Build Directory and Reconfigure" command` - `Improve CTest test ordering to match Test Explorer display` **Bad examples:** - `Fix bug` (too vague) - `Update code` (no useful information) - `WIP` (not ready for review) ### 2. PR Description The PR body must include: - **What changed**: A short summary of the user-visible behavior change. - **Why**: The motivation — link to a GitHub issue if one exists (e.g., `Fixes #1234`). - **How** (if non-obvious): A brief explanation of the implementation approach when the change is complex. ### 3. CHANGELOG Entry Every PR must add an entry to `CHANGELOG.md`. #### Where to insert Insert the entry under the **most recent (topmost) version heading** in `CHANGELOG.md`. The first version heading looks like `## <version>` (e.g., `## 1.23`). Always add the new entry at the **bottom** of the appropriate section (i.e., after all existing entries in that section). #### Which section Place the entry in exactly one of these three sections, creating the section if it does not already exist under the current version: | Section | Use when… | |---|---| | `Features:` | A new user-visible capability is added (new command, new setting, new UI element). | | `Improvements:` | An existing feature is enhanced, optimized, or has better UX — but no new capability is introduced. | | `Bug Fixes:` | A defect is corrected. | The sections appear in this fixed order: `Features:`, then `Improvements:`, then `Bug Fixes:`. #### Entry format Each entry follows this pattern: ``` - <Description>. [#<number>](<link>) ``` Where `<Description>` starts with a present-tense verb describing the user-visible change, and the link references either: - The **GitHub issue** it solves: `[#<issue number>](https://github.com/microsoft/vscode-cmake-tools/issues/<issue number>)` - Or the **PR** itself: `[#<pr number>](https://github.com/microsoft/vscode-cmake-tools/pull/<pr number>)` An entry may optionally credit an external contributor at the end: `[@user](https://github.com/user)`. **Examples:** ```markdown Features: - Add "Delete Build Directory and Reconfigure" command that removes the entire build directory before reconfiguring, ensuring a completely clean state. [#4826](https://github.com/microsoft/vscode-cmake-tools/pull/4826) Improvements: - Run tests sequentially in alphabetical order (matching the Test Explorer display order) when `cmake.ctest.allowParallelJobs` is disabled. [#4829](https://github.com/microsoft/vscode-cmake-tools/issues/4829) Bug Fixes: - Fix `cmake.revealLog` set to `"focus"` not revealing the output panel or stealing focus. [#4471](https://github.com/microsoft/vscode-cmake-tools/issues/4471) - Fix garbled characters in the Output panel when MSVC outputs UTF-8 on non-UTF-8 Windows systems. [#4520](https://github.com/microsoft/vscode-cmake-tools/issues/4520) [@contributor](https://github.com/contributor) ``` #### What NOT to do - Do **not** add a new version heading — use the existing topmost one. - Do **not** place the entry under an older version. - Do **not** use past tense (write "Fix …", not "Fixed …"). - Do **not** omit the issue or PR link. ### 4. Correctness Review the code changes for logical correctness: - **Both operating modes**: If the change touches shared logic (configure, build, test, targets, environment), verify it handles both *presets mode* and *kits/variants mode*. Check for `useCMakePresets` branching where appropriate. - **Both generator types**: If the change involves build-type logic, verify it handles both single-config generators (`CMAKE_BUILD_TYPE` at configure time) and multi-config generators (`--config` at build time). - **Edge cases**: Look for off-by-one errors, null/undefined access, missing `await` on async calls, and unhandled promise rejections. - **Error handling**: Verify errors are not silently swallowed. Top-level event handlers should use `rollbar.invokeAsync()` / `rollbar.invoke()`. Empty `catch` blocks are a red flag. - **Cross-platform**: Check for hardcoded path separators (`/` or `\\`), case-sensitive env var assumptions, or platform-specific APIs used without guards. Paths must use `path.join()` / `path.normalize()`. ### 5. Regression Risks Identify areas where the change could break existing behavior: - **Shared utilities**: Changes to `src/expand.ts`, `src/proc.ts`, `src/shlex.ts`, or `src/util.ts` affect many callers — verify all call sites still behave correctly. - **Driver base class**: Changes to `cmakeDriver.ts` propagate to `cmakeFileApiDriver.ts`, `cmakeLegacyDriver.ts`, and `cmakeServerDriver.ts`. Check that subclass overrides are still compatible. - **Preset merging**: Changes to `presetsController.ts` or `presetsParser.ts` can alter how presets resolve — verify with nested `include` chains and `CMakeUserPresets.json` overrides. - **Settings**: Adding or renaming a setting in `package.json` without updating `src/config.ts` (or vice versa) causes silent failures. - **Task provider**: Changes to `cmakeTaskProvider.ts` can break `tasks.json` definitions that users have already configured. - **Public API / extensibility**: Changes to exports in `src/api.ts` or types in `EXTENSIBILITY.md` can break dependent extensions. - **Test coverage**: Flag changes to critical paths that lack corresponding test updates, especially in `src/drivers/`, `src/presets/`, and `src/kits/`. See also section 7 (Test Coverage) for detailed test review guidance. ### 6. Adherence to Existing Patterns Verify the change follows the project's established conventions: - **Import style**: Uses `@cmt/*` path aliases (not relative paths from outside `src/`). Uses `import * as nls from 'vscode-nls'` for localization. - **Logging**: Uses `logging.createLogger('module-name')` — never `console.log`. - **Localization**: All user-visible strings use `localize('message.key', 'Message text')` with the `vscode-nls` boilerplate at the top of the file. - **Settings access**: Reads settings through `ConfigurationReader` (`src/config.ts`) — never calls `vscode.workspace.getConfiguration()` directly. - **Telemetry**: Uses helpers from `src/telemetry.ts` — never calls the VS Code telemetry API directly. - **Data access**: Uses canonical data paths (e.g., `CMakeProject.targets` for targets, `CMakeDriver.cmakeCacheEntries` for cache) — never parses CMake files or cache directly. - **Async patterns**: Prefers `async`/`await` over `.then()` chains (exception: fire-and-forget UI calls). - **Naming and structure**: New files are placed in the correct layer directory (see architecture table in `.github/copilot-instructions.md`). New commands are registered in `src/extension.ts`. ### 7. Test Coverage Verify that the PR includes adequate tests for the changes: - **New functionality**: Any new function, command, setting, or behavior branch should have corresponding tests. If the change adds a new helper or utility, check that it has unit tests covering its inputs and edge cases. - **Bug fixes**: A bug fix should include at least one test that would have caught the original bug — i.e., a test that fails on the base branch and passes with the fix applied. - **Changed behavior**: If existing behavior is modified, check that existing tests are updated to reflect the new behavior and that no tests are silently broken or deleted without justification. - **Test location**: Unit tests that don't require VS Code APIs belong in `test/unit-tests/backend/` and run via `yarn backendTests`. Tests that require VS Code belong in `test/unit-tests/` and run via `yarn unitTests`. Check that new tests are placed in the correct location. - **Test quality**: Tests should be specific and descriptive: - Test names should clearly state what is being tested and the expected outcome. - Tests should assert specific values, not just "no error thrown". - Tests should cover both positive cases (expected input → expected output) and negative/edge cases (invalid input, boundary conditions, fallback paths). - Comments in tests should be accurate and not misleading about what the test actually exercises. - **Coverage gaps to flag**: Pay special attention to: - Code paths that touch `src/drivers/`, `src/presets/`, or `src/kits/` — these are critical and should have test coverage for any non-trivial changes. - Fallback/error paths — if the PR adds a fallback (e.g., "if X fails, try Y"), both the success and fallback paths should be tested. - Platform-specific branches — if the code branches on `process.platform`, each branch should ideally be covered. - **When tests are not feasible**: Some changes (e.g., pure UI wiring, VS Code API integration) are difficult to unit test. In these cases, verify that the manual-checklist or PR description explains how to manually verify the change, and flag the gap rather than ignoring it. ### 8. Documentation Updates Check whether the change requires documentation updates: - **New or changed settings**: Must be reflected in all three locations — `package.json` (`contributes.configuration`), `src/config.ts` (`ConfigurationReader`), and `docs/cmake-settings.md`. - **New commands**: Must be documented in `package.json` (`contributes.commands`) and referenced in the relevant docs page under `docs/`. - **User-visible behavior changes**: If the change alters how a feature works (not just fixes a bug), check whether `docs/` pages describing that feature need updating. - **Extensibility changes**: If `src/api.ts` or public types change, update `EXTENSIBILITY.md`. - **README**: If a new major feature is added, check whether `README.md` should mention it. ## Applying This Skill When reviewing or preparing a PR: 1. **Check the title** — rewrite it if it is vague or missing context. 2. **Check the description** — ensure it explains what, why, and (if needed) how. 3. **Check `CHANGELOG.md`** — verify an entry exists under the current version in the correct section with the correct format. If missing, add one. 4. **Check correctness** — review code for logical errors, missing mode/generator handling, cross-platform issues, and error handling gaps. 5. **Check regression risks** — identify areas where the change could break existing behavior and flag missing test coverage for critical paths. 6. **Check pattern adherence** — verify the change follows established import, logging, localization, settings access, and architectural conventions. 7. **Check test coverage** — verify that new or changed behavior has corresponding tests, bug fixes include regression tests, and tests are placed in the correct location with good quality. 8. **Check documentation** — verify that new or changed settings, commands, and behavior are reflected in the appropriate docs.