style: enforce quoted literals, named parameters, validation; pin LF endings#23
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR strengthens the codebase's build infrastructure and test robustness by enforcing LF line endings globally, adding parameter validation to PowerShell functions, refactoring path construction in test setup code, and standardizing string quoting in verbose logging. ChangesConfiguration & Build Settings
Test Infrastructure Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the PowerShell module template to better align in-tree files with the existing PowerShell style guidance (quoted literals, named parameters, parameter validation) and to normalize repository line endings to LF to avoid CRLF churn/warnings for Windows contributors.
Changes:
- Enforce LF line endings repo-wide via
.gitattributes. - Add/strengthen parameter validation attributes in build/test helper functions.
- Replace several positional cmdlet arguments with named parameters and normalize string literal quoting in template/test files.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
.gitattributes |
Forces LF EOL normalization across the repo. |
build.ps1 |
Adds [ValidateNotNullOrEmpty()] to $Task. |
{{ModuleName}}/Public/Get-{{Prefix}}Example.ps1 |
Normalizes Write-Verbose string literals to single quotes. |
tests/Help.tests.ps1 |
Converts several positional cmdlet usages to named parameters (path/module name). |
tests/Manifest.tests.ps1 |
Uses named parameters for path/content operations; normalizes -ErrorAction quoting. |
tests/Meta.tests.ps1 |
Uses named parameters for MetaFixers helper calls and Select-String. |
tests/MetaFixers.psm1 |
Adds validation attributes and uses named -FileInfo in Test-FileUnicode call site. |
tests/ManifestHelpers.psm1 |
Adds validation attributes, normalizes throws to single quotes, uses named -VersionString. |
tests/Unit/Public/Get-{{Prefix}}Example.tests.ps1 |
Uses named parameters for nested Split-Path, Join-Path, and Get-Module. |
tests/Unit/Private/Invoke-{{Prefix}}Helper.tests.ps1 |
Same as above; also switches InModuleScope to named parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ManifestHelpers.psm1`:
- Around line 39-45: The explicit runtime check using
[string]::IsNullOrEmpty($VersionString) is dead code because the parameter
already has the [ValidateNotNullOrEmpty()] attribute; remove the if-block that
throws 'VersionString cannot be empty or null' to eliminate redundancy, or if
you need to also reject whitespace-only strings replace the attribute with
[ValidateNotNullOrWhiteSpace()] and then remove the manual IsNullOrEmpty check;
reference the parameter $VersionString and the attribute
[ValidateNotNullOrEmpty()] / the existing IsNullOrEmpty check when applying the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d9ab530-d123-4428-9c41-4f4aee8628b8
📒 Files selected for processing (10)
.gitattributesbuild.ps1tests/Help.tests.ps1tests/Manifest.tests.ps1tests/ManifestHelpers.psm1tests/Meta.tests.ps1tests/MetaFixers.psm1tests/Unit/Private/Invoke-{{Prefix}}Helper.tests.ps1tests/Unit/Public/Get-{{Prefix}}Example.tests.ps1{{ModuleName}}/Public/Get-{{Prefix}}Example.ps1
…ation
The repo's own powershell.instructions.md mandates three rules for
in-tree code:
- literal string parameter values are single-quoted
- cmdlet calls use named parameters (when more than one arg is passed)
- every function parameter has an appropriate validator
This commit applies those rules to the template's first-party files —
the example public/private functions, the test scaffolding, the meta
helper modules, and build.ps1. Notable changes:
- build.ps1 gains [ValidateNotNullOrEmpty()] on $Task
- tests/MetaFixers.psm1 + tests/ManifestHelpers.psm1 gain validators
on every parameter that didn't already have one
- tests/Help.tests.ps1, tests/Manifest.tests.ps1, tests/Meta.tests.ps1
rename positional Split-Path / Join-Path / Get-Module / Get-Content
calls to use named -Path / -ChildPath / -Name / -Pattern parameters
- the example Get-{{Prefix}}Example.ps1 / its tests get the same
treatment so newly-scaffolded modules start out compliant
No behavioural changes. CI on the un-initialized template still skips
build/test (the {{GUID}} placeholder is unparseable until init runs);
parse-checks confirm every edited file is syntactically valid.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The repo already stores every text file as LF in the index, but Windows clones with global core.autocrlf=true rewrite the working tree to CRLF on checkout. Subsequent edits made through tooling that writes LF (most editors and AI agents do) trigger 'LF will be replaced by CRLF the next time Git touches it' warnings on every commit. Pinning eol=lf in .gitattributes overrides the contributor's global autocrlf setting for this repo and silences the warning. PowerShell on Windows handles LF fine, and the devcontainer already runs Linux. NOTE: GitHub template repos are one-time copies at creation, so this fix does NOT propagate to repositories already created from this template. Existing downstream repos must apply the same .gitattributes change separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added [ValidateNotNullOrEmpty()] to $VersionString, which makes the in-body IsNullOrEmpty check unreachable under normal parameter binding — both Copilot and CodeRabbit flagged it as dead code. Removing the redundant guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b16acbd to
abf5d46
Compare
Summary
Two concerns, both fixing first-party files in the template so newly-scaffolded modules inherit clean defaults:
instructions/powershell.instructions.mdalready mandates these for in-tree code, but several files don't follow them yet:.gitattributesnow pins everything toeol=lfso Windows clones withcore.autocrlf=truestop emitting theLF will be replaced by CRLFwarning on every commit.No behavioural changes.
What changed
.gitattributes* text=auto eol=lf— overrides contributor's globalcore.autocrlffor this repobuild.ps1[ValidateNotNullOrEmpty()]on$Task{{ModuleName}}/Public/Get-{{Prefix}}Example.ps1Write-Verboseliterals single-quoted (kept positional — single arg)tests/Help.tests.ps1Split-Path/Join-Path/Get-Module/Test-Pathrewritten to use named-Path/-ChildPath/-Nametests/Manifest.tests.ps1Split-Path/Join-Path/Get-Contentrewritten to named;-ErrorAction Stopliteral single-quotedtests/Meta.tests.ps1Get-TextFilesList/Test-FileUnicode/Get-Content/Select-Stringrewritten to use the named parameters their declarations expose (-Root,-FileInfo,-Path,-Pattern)tests/MetaFixers.psm1[ValidateNotNull()]onFileInfoparameters;[ValidateNotNullOrEmpty()]onRoot; one positionalTest-FileUnicode $_rewritten to-FileInfo $_tests/ManifestHelpers.psm1[ValidateNotNullOrEmpty()]on the eight mandatory string params; positionalSplit-SemVerStringcalls switched to named-VersionString; twothrow "..."literals switched to single quotestests/Unit/Public/Get-{{Prefix}}Example.tests.ps1Split-Path -Parent <var>switched toSplit-Path -Path <var> -Parent;Join-Pathnamed;Get-Modulenamedtests/Unit/Private/Invoke-{{Prefix}}Helper.tests.ps1InModuleScope $name { ... }switched toInModuleScope -ModuleName $name -ScriptBlock { ... }GitHub template repos are one-time copies at creation — when someone clicks "Use this template," GitHub copies the files at that moment and the new repo has its own independent commit history. Subsequent updates to this template do not auto-propagate to repositories created from it.
That means every existing repo created from this template still has the same problems and needs the equivalent fix applied separately:
.gitattributeseol=lfline — same one-line addition, thengit add --renormalize . && git commit.Repos affected (non-fork, PowerShell-language repos under
tablackburn):JsmOperations—eol=lfalready applied; style fixes already merged equivalents onfeature/v0.1.0-mvpPlexAutomationToolkitReScenePSSrrDBAutomationToolkitYouTubeMusicPSScheduledTasksManagerai-agent-instruction-modulesPoSHTinyTinyRSSA follow-up sweep should apply the same
.gitattributeschange to the eight that don't have it.Verification
Initialize-Template.ps1runs (the manifest contains literal{{GUID}}), and CI explicitly skips lint/test in the template state. So I couldn't run./build.ps1here.[Parser]::ParseFile(raw) or[Parser]::ParseInput(placeholder files with stubs substituted) — all clean.tablackburn/JsmOperationsmodule already runs equivalent edits through Pester + PSScriptAnalyzer + 100% coverage on every commit; it serves as de facto validation that the patterns work end-to-end.Out of scope
Initialize-Template.ps1itself — also has positional calls and missing validators, but it's a one-shot init script with very different shape and risk profile. Worth a follow-up PR if you want to apply the same rules there.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores