Skip to content

Commit 69caa84

Browse files
cnaples79WilliamBerryiii
cnaples79
and
WilliamBerryiii
authored
refactor: remove duplicate git diff logic in frontmatter validator (#473)
Removes duplicate `Get-ChangedMarkdownFileGroup` function from the frontmatter validator, replacing it with the shared `Get-ChangedFilesFromGit` helper from `LintingHelpers.psm1`. This eliminates 491 lines of duplicate code. - Removed 104-line duplicate function from `Validate-MarkdownFrontmatter.ps1`, replaced with shared helper call - Removed 386 lines of obsolete test code for the deleted function - Updated integration test mocks to use `Get-ChangedFilesFromGit` ## Related Issue(s) Fixes #322 ## Type of Change Select all that apply: **Code & Documentation:** - [ ] Bug fix (non-breaking change fixing an issue) - [ ] New feature (non-breaking change adding functionality) - [ ] Breaking change (fix or feature causing existing functionality to change) - [ ] Documentation update **Infrastructure & Configuration:** - [ ] GitHub Actions workflow - [ ] Linting configuration (markdown, PowerShell, etc.) - [ ] Security configuration - [ ] DevContainer configuration - [ ] Dependency update **AI Artifacts:** - [ ] Reviewed contribution with `prompt-builder` agent and addressed all feedback - [ ] Copilot instructions (`.github/instructions/*.instructions.md`) - [ ] Copilot prompt (`.github/prompts/*.prompt.md`) - [ ] Copilot agent (`.github/agents/*.agent.md`) - [ ] Copilot skill (`.github/skills/*/SKILL.md`) **Other:** - [x] Script/automation (`.ps1`, `.sh`, `.py`) - [ ] Other (please describe): ## Testing - All CI checks passing - PowerShell tests pass - Shared function already has test coverage in `LintingHelpers.Tests.ps1` ## Checklist ### Required Checks - [ ] Documentation is updated (if applicable) - [ ] Files follow existing naming conventions - [ ] Changes are backwards compatible (if applicable) - [ ] Tests added for new functionality (if applicable) ### Required Automated Checks The following validation commands must pass before merging: - [ ] Markdown linting: `npm run lint:md` - [ ] Spell checking: `npm run spell-check` - [ ] Frontmatter validation: `npm run lint:frontmatter` - [ ] Skill structure validation: `npm run validate:skills` - [ ] Link validation: `npm run lint:md-links` - [ ] PowerShell analysis: `npm run lint:ps` ## Security Considerations - [ ] This PR does not contain any sensitive or NDA information - [ ] Any new dependencies have been reviewed for security issues - [ ] Security-related scripts follow the principle of least privilege ## Additional Notes Files changed: - `scripts/linting/Validate-MarkdownFrontmatter.ps1` (-106 lines) - `scripts/tests/linting/Validate-MarkdownFrontmatter.Tests.ps1` (-386 lines) --------- Co-authored-by: Bill Berry <WilliamBerryiii@users.noreply.github.com> Co-authored-by: Bill Berry <wberry@microsoft.com>
1 parent 975e862 commit 69caa84

File tree

3 files changed

+139
-491
lines changed

3 files changed

+139
-491
lines changed

‎scripts/linting/Validate-MarkdownFrontmatter.ps1‎

Lines changed: 1 addition & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ function Test-FrontmatterValidation {
706706
# Handle ChangedFilesOnly mode
707707
if ($ChangedFilesOnly) {
708708
Write-Host "🔍 Detecting changed markdown files from git diff..." -ForegroundColor Cyan
709-
$Files = @(Get-ChangedMarkdownFileGroup -BaseBranch $BaseBranch)
709+
$Files = @(Get-ChangedFilesFromGit -BaseBranch $BaseBranch -FileExtensions @('*.md'))
710710
if (@($Files).Count -eq 0) {
711711
Write-Host "No changed markdown files found - validation complete" -ForegroundColor Green
712712
# Return empty summary with TotalFiles=0 to accurately represent no files validated
@@ -833,110 +833,6 @@ All frontmatter fields are valid and properly formatted. Great job! 🎉
833833
return $summary
834834
}
835835

836-
function Get-ChangedMarkdownFileGroup {
837-
<#
838-
.SYNOPSIS
839-
Retrieves changed markdown files from git diff comparison.
840-
841-
.DESCRIPTION
842-
Uses git diff to identify markdown files that have changed between the current
843-
HEAD and a base branch. Implements a fallback strategy when standard comparison
844-
methods fail:
845-
846-
1. First attempts: git merge-base comparison with specified base branch
847-
2. Fallback 1: Comparison with HEAD~1 (previous commit)
848-
3. Fallback 2: Staged and unstaged files against HEAD
849-
850-
.PARAMETER BaseBranch
851-
Git reference for the base branch to compare against. Defaults to 'origin/main'.
852-
Can be any valid git ref (branch name, tag, commit SHA).
853-
854-
.PARAMETER FallbackStrategy
855-
Controls fallback behavior when primary comparison fails.
856-
- 'Auto' (default): Tries all fallback strategies automatically
857-
- 'HeadOnly': Only uses HEAD~1 fallback
858-
- 'None': No fallback, returns empty on failure
859-
860-
.INPUTS
861-
None. Does not accept pipeline input.
862-
863-
.OUTPUTS
864-
[string[]] Array of relative file paths for changed markdown files.
865-
Returns empty array if no changes detected or git operations fail.
866-
867-
.EXAMPLE
868-
$changedFiles = Get-ChangedMarkdownFileGroup
869-
# Returns markdown files changed compared to origin/main
870-
871-
.EXAMPLE
872-
$changedFiles = Get-ChangedMarkdownFileGroup -BaseBranch 'origin/develop'
873-
# Returns markdown files changed compared to develop branch
874-
875-
.EXAMPLE
876-
$changedFiles = Get-ChangedMarkdownFileGroup -FallbackStrategy 'None'
877-
# Returns empty array if merge-base comparison fails
878-
879-
.NOTES
880-
Requires git to be available in PATH. Files must exist on disk to be included
881-
in the result (deleted files are excluded).
882-
#>
883-
[CmdletBinding()]
884-
[OutputType([string[]])]
885-
param(
886-
[Parameter(Mandatory = $false, Position = 0)]
887-
[ValidateNotNullOrEmpty()]
888-
[string]$BaseBranch = "origin/main",
889-
890-
[Parameter(Mandatory = $false)]
891-
[ValidateSet('Auto', 'HeadOnly', 'None')]
892-
[string]$FallbackStrategy = 'Auto'
893-
)
894-
895-
try {
896-
$changedFiles = git diff --name-only $(git merge-base HEAD $BaseBranch) HEAD 2>$null
897-
if ($LASTEXITCODE -ne 0) {
898-
Write-Verbose "Merge base comparison with '$BaseBranch' failed"
899-
900-
if ($FallbackStrategy -eq 'None') {
901-
Write-Warning "Unable to determine changed files from git (no fallback enabled)"
902-
return @()
903-
}
904-
905-
Write-Verbose "Attempting fallback: HEAD~1 comparison"
906-
$changedFiles = git diff --name-only HEAD~1 HEAD 2>$null
907-
908-
if ($LASTEXITCODE -ne 0 -and $FallbackStrategy -eq 'Auto') {
909-
Write-Verbose "HEAD~1 comparison failed, attempting staged/unstaged files"
910-
$changedFiles = git diff --name-only HEAD 2>$null
911-
912-
if ($LASTEXITCODE -ne 0) {
913-
Write-Warning "Unable to determine changed files from git"
914-
return @()
915-
}
916-
}
917-
elseif ($LASTEXITCODE -ne 0) {
918-
Write-Warning "Unable to determine changed files from git"
919-
return @()
920-
}
921-
}
922-
923-
[string[]]$changedMarkdownFiles = $changedFiles | Where-Object {
924-
-not [string]::IsNullOrEmpty($_) -and
925-
$_ -match '\.md$' -and
926-
(Test-Path $_ -PathType Leaf)
927-
}
928-
929-
Write-Verbose "Found $($changedMarkdownFiles.Count) changed markdown files from git diff"
930-
$changedMarkdownFiles | ForEach-Object { Write-Verbose " Changed: $_" }
931-
932-
return $changedMarkdownFiles
933-
}
934-
catch {
935-
Write-Warning "Error getting changed files from git: $($_.Exception.Message)"
936-
return @()
937-
}
938-
}
939-
940836
#region Main Execution
941837
if ($MyInvocation.InvocationName -ne '.') {
942838
try {

‎scripts/tests/linting/LintingHelpers.Tests.ps1‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,134 @@ Describe 'Get-ChangedFilesFromGit' {
220220
$result | Should -BeNullOrEmpty
221221
}
222222
}
223+
224+
Context 'Warning and verbose output' {
225+
It 'Emits warning when git diff returns non-zero exit code' {
226+
Mock git {
227+
$global:LASTEXITCODE = 0
228+
return 'abc123def456789'
229+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
230+
231+
Mock git {
232+
$global:LASTEXITCODE = 1
233+
return $null
234+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'diff' }
235+
236+
$output = Get-ChangedFilesFromGit 3>&1
237+
$warnings = @($output | Where-Object { $_ -is [System.Management.Automation.WarningRecord] })
238+
$warnings | Should -Not -BeNullOrEmpty
239+
}
240+
241+
It 'Emits warning when exception occurs' {
242+
Mock git {
243+
throw "Simulated git failure"
244+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
245+
246+
$warnings = Get-ChangedFilesFromGit 3>&1 | Where-Object { $_ -is [System.Management.Automation.WarningRecord] }
247+
$warnings | Should -Not -BeNullOrEmpty
248+
}
249+
250+
It 'Emits verbose message when merge-base succeeds' {
251+
Mock git {
252+
$global:LASTEXITCODE = 0
253+
return 'abc123def456789'
254+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
255+
256+
Mock git {
257+
$global:LASTEXITCODE = 0
258+
return @('file.ps1')
259+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'diff' }
260+
261+
Mock Test-Path { return $true } -ModuleName 'LintingHelpers' -ParameterFilter { $PathType -eq 'Leaf' }
262+
263+
$verbose = Get-ChangedFilesFromGit -Verbose 4>&1 | Where-Object { $_ -is [System.Management.Automation.VerboseRecord] }
264+
$verbose | Should -Not -BeNullOrEmpty
265+
}
266+
267+
It 'Emits verbose message when falling back to HEAD~1' {
268+
Mock git {
269+
$global:LASTEXITCODE = 128
270+
return $null
271+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
272+
273+
Mock git {
274+
$global:LASTEXITCODE = 0
275+
return 'HEAD~1-sha'
276+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'rev-parse' }
277+
278+
Mock git {
279+
$global:LASTEXITCODE = 0
280+
return @('file.ps1')
281+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'diff' }
282+
283+
Mock Test-Path { return $true } -ModuleName 'LintingHelpers' -ParameterFilter { $PathType -eq 'Leaf' }
284+
285+
$verbose = Get-ChangedFilesFromGit -Verbose 4>&1 | Where-Object { $_ -is [System.Management.Automation.VerboseRecord] }
286+
$verbose | Should -Not -BeNullOrEmpty
287+
($verbose | Where-Object { $_.Message -match 'HEAD~1' }) | Should -Not -BeNullOrEmpty
288+
}
289+
}
290+
291+
Context 'Custom BaseBranch parameter' {
292+
BeforeEach {
293+
Mock git {
294+
$global:LASTEXITCODE = 0
295+
return 'abc123def456789'
296+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
297+
298+
Mock git {
299+
$global:LASTEXITCODE = 0
300+
return @('file.md')
301+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'diff' }
302+
303+
Mock Test-Path { return $true } -ModuleName 'LintingHelpers' -ParameterFilter { $PathType -eq 'Leaf' }
304+
}
305+
306+
It 'Passes custom BaseBranch to merge-base' {
307+
Get-ChangedFilesFromGit -BaseBranch 'origin/develop' -FileExtensions @('*.md')
308+
Should -Invoke git -ModuleName 'LintingHelpers' -ParameterFilter {
309+
$args[0] -eq 'merge-base' -and $args -contains 'origin/develop'
310+
}
311+
}
312+
313+
It 'Uses default BaseBranch when not specified' {
314+
Get-ChangedFilesFromGit -FileExtensions @('*.md')
315+
Should -Invoke git -ModuleName 'LintingHelpers' -ParameterFilter {
316+
$args[0] -eq 'merge-base' -and $args -contains 'origin/main'
317+
}
318+
}
319+
}
320+
321+
Context 'Mixed path separators' {
322+
BeforeEach {
323+
Mock git {
324+
$global:LASTEXITCODE = 0
325+
return 'abc123def456789'
326+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'merge-base' }
327+
328+
Mock git {
329+
$global:LASTEXITCODE = 0
330+
return @('src/docs/readme.md', 'src\tests\test.md', 'docs/guide.md')
331+
} -ModuleName 'LintingHelpers' -ParameterFilter { $args[0] -eq 'diff' }
332+
333+
Mock Test-Path { return $true } -ModuleName 'LintingHelpers' -ParameterFilter { $PathType -eq 'Leaf' }
334+
}
335+
336+
It 'Handles files with forward slashes' {
337+
$result = Get-ChangedFilesFromGit -FileExtensions @('*.md')
338+
$result | Should -Contain 'src/docs/readme.md'
339+
}
340+
341+
It 'Handles files with backslashes' {
342+
$result = Get-ChangedFilesFromGit -FileExtensions @('*.md')
343+
$result | Should -Contain 'src\tests\test.md'
344+
}
345+
346+
It 'Returns correct count with mixed separators' {
347+
$result = Get-ChangedFilesFromGit -FileExtensions @('*.md')
348+
$result.Count | Should -Be 3
349+
}
350+
}
223351
}
224352

225353
#endregion

0 commit comments

Comments
 (0)