Refactor duplicated code into shared utilities module - #23
Refactor duplicated code into shared utilities module#23devin-ai-integration[bot] wants to merge 1 commit into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; all delegating wrappers are behaviorally identical to the originals and the safety-critical paths (policy blocking, preset cycle detection, registry WhatIf) are unaffected. The consolidation is mechanical — every shared implementation is a direct copy of the original, and all public wrappers forward arguments unchanged. The only new issue is a backslash literal inside a Join-Path child argument in Invoke-BraveDebloat.ps1 that would break the dot-source on macOS/Linux; on Windows (the primary target) it works correctly. Invoke-BraveDebloat.ps1 line 61 — the Join-Path call for Shared.ps1 uses a backslash in the child argument. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Invoke-BraveDebloat.ps1] -->|dot-source| S[scripts/Shared.ps1]
B[scripts/Test-PolicyManifest.ps1] -->|dot-source| S
C[scripts/Test-Behavior.ps1] -->|dot-source| S
S --> F1[ConvertTo-PropertyMap]
S --> F2[Resolve-PresetEntries]
S --> F3[Get-PolicySafetyFindings]
S --> F4[Format-TableOutput]
S --> F5[Assert-TextContains / Assert-TextDoesNotContain]
A -->|dot-source| M1[src/Common.ps1]
A -->|dot-source| M2[src/Manifest.ps1]
A -->|dot-source| M3[src/Reports.ps1]
M1 -->|thin wrapper| F1
M2 -->|thin wrappers| F2
M2 -->|thin wrappers| F3
M3 -->|calls| F4
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Invoke-BraveDebloat.ps1] -->|dot-source| S[scripts/Shared.ps1]
B[scripts/Test-PolicyManifest.ps1] -->|dot-source| S
C[scripts/Test-Behavior.ps1] -->|dot-source| S
S --> F1[ConvertTo-PropertyMap]
S --> F2[Resolve-PresetEntries]
S --> F3[Get-PolicySafetyFindings]
S --> F4[Format-TableOutput]
S --> F5[Assert-TextContains / Assert-TextDoesNotContain]
A -->|dot-source| M1[src/Common.ps1]
A -->|dot-source| M2[src/Manifest.ps1]
A -->|dot-source| M3[src/Reports.ps1]
M1 -->|thin wrapper| F1
M2 -->|thin wrappers| F2
M2 -->|thin wrappers| F3
M3 -->|calls| F4
Reviews (2): Last reviewed commit: "Refactor duplicated code into shared uti..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function Assert-TextContains { | ||
| param( | ||
| [Parameter(Mandatory = $true)][string]$Text, | ||
| [Parameter(Mandatory = $true)][string]$Expected, | ||
| [Parameter(Mandatory = $true)][string]$Context | ||
| ) | ||
|
|
||
| if (-not $Text.Contains($Expected)) { | ||
| throw "$Context did not contain expected text: $Expected" | ||
| } | ||
| } | ||
|
|
||
| function Assert-TextDoesNotContain { | ||
| param( | ||
| [Parameter(Mandatory = $true)][string]$Text, | ||
| [Parameter(Mandatory = $true)][string]$Unexpected, | ||
| [Parameter(Mandatory = $true)][string]$Context | ||
| ) | ||
|
|
||
| if ($Text.Contains($Unexpected)) { | ||
| throw "$Context contained unexpected text: $Unexpected" | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert-TextContains and Assert-TextDoesNotContain are test-only assertions. Because Shared.ps1 is dot-sourced by the production script Invoke-BraveDebloat.ps1, these functions now exist in that script's function namespace. They have no caller in production code and create unnecessary surface area that could confuse future contributors looking at what functions the main script exposes. A cleaner split would keep test-only helpers in a separate scripts/Test-Shared.ps1 (or inline only in Test-Behavior.ps1, their sole consumer) and restrict Shared.ps1 to functions used by both production and test code.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Extract repeated patterns into scripts/Shared.ps1: - ConvertTo-PropertyMap: object-to-hashtable conversion (was Get-ManifestMap / Get-ObjectMap) - Resolve-PresetEntries: recursive preset resolution with cycle detection - Get-PolicySafetyFindings: unified policy safety validation with -Throw switch - Format-TableOutput: table display formatting wrapper - Assert-TextContains / Assert-TextDoesNotContain: test assertion helpers src/Common.ps1 Get-ManifestMap now delegates to ConvertTo-PropertyMap. src/Manifest.ps1 Resolve-PresetPolicies delegates to Resolve-PresetEntries; Assert-PolicySafety and Get-PolicySafetyFinding delegate to Get-PolicySafetyFindings. src/Reports.ps1 uses Format-TableOutput instead of inline Format-Table calls. Test scripts source Shared.ps1 and drop their local duplicates. Co-Authored-By: adasdasd dasdasdas <fearlemail@gmail.com>
2c13709 to
0a26b59
Compare
1 participant
Summary
Extracts 5 duplicated code patterns into
scripts/Shared.ps1, sourced byInvoke-BraveDebloat.ps1beforesrc/modules and independently by test scripts:Original function names (
Get-ManifestMap,Resolve-PresetPolicies,Assert-PolicySafety,Get-PolicySafetyFinding) remain as thin delegation wrappers — public API unchanged.Net: +137 / −145 lines.
Link to Devin session: https://app.devin.ai/sessions/9f147750e5374db88e58f640d07492cd
Requested by: @osfv