[fp-enhancer] Improve pkg/cli functional/immutability patterns#16788
[fp-enhancer] Improve pkg/cli functional/immutability patterns#16788github-actions[bot] wants to merge 2 commits intomainfrom
Conversation
- sanitizeValidationResults: use sliceutil.Map for outer and inner loops, extract pure helper function sanitizeCompileValidationError - buildAuditData: replace var+append jobs loop with sliceutil.Map - generateFailureAnalysis: replace var+append failedJobs loop with sliceutil.FilterMap These changes eliminate mutable intermediate state and make data transformations more declarative, improving clarity and reducing mutation surface area. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@copilot merge main |
There was a problem hiding this comment.
Pull request overview
This PR mixes two unrelated concerns: functional programming improvements to pkg/cli and Playwright domain support for pkg/workflow. The PR description only mentions the functional programming changes but omits significant changes including Playwright domain support, workflow lock file updates, action pin removals, and documentation additions.
Changes:
- Refactored three functions in
pkg/clito usesliceutil.Mapandsliceutil.FilterMapfor more declarative data transformations - Added Playwright ecosystem domain support to automatically allow required domains when Playwright tool is configured
- Removed 6 unused action pin entries and updated stale-repos action SHA
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/compile_config.go | Refactored sanitizeValidationResults to use sliceutil.Map and extracted pure helper sanitizeCompileValidationError |
| pkg/cli/audit_report.go | Replaced imperative loop with sliceutil.Map for building job data |
| pkg/cli/audit_report_analysis.go | Replaced filter+append pattern with sliceutil.FilterMap for collecting failed job names |
| pkg/workflow/domains.go | Added PlaywrightDomains constant and extractPlaywrightDomains function to support Playwright browser downloads |
| pkg/workflow/http_mcp_domains_test.go | Added comprehensive test coverage for Playwright domain extraction |
| .github/workflows/stale-repo-identifier.lock.yml | Updated stale-repos action SHA with mismatched version comment |
| .github/workflows/slide-deck-maintainer.lock.yml | Added Playwright domains to allow-domains list |
| .github/workflows/docs-noob-tester.lock.yml | Added Playwright domains to allow-domains list |
| pkg/workflow/data/action_pins.json | Removed 6 unused action pin entries (stale-repos@v3.0.2, haskell-actions/setup@v2.10.3, oven-sh/setup-bun@v2.1.2, ruby/setup-ruby@v1.288.0, super-linter/super-linter@v8.2.1) |
| .github/aw/actions-lock.json | Removed same 6 unused action pin entries |
| docs/src/content/docs/reference/glossary.md | Added Playground glossary entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // PlaywrightDomains are the domains required for Playwright browser downloads | ||
| // These domains are needed when Playwright MCP server initializes in the Docker container | ||
| var PlaywrightDomains = []string{ | ||
| "cdn.playwright.dev", | ||
| "playwright.download.prss.microsoft.com", | ||
| } |
There was a problem hiding this comment.
Major discrepancy between PR description and actual changes
The PR title and description claim this PR improves "pkg/cli functional/immutability patterns", but the majority of changes are unrelated to this stated purpose:
-
pkg/workflow changes (NOT mentioned in description):
- New Playwright domain support in
domains.go - New test cases in
http_mcp_domains_test.go
- New Playwright domain support in
-
Lock file changes (NOT mentioned in description):
- Removal of 6 action pin entries in
action_pins.jsonandactions-lock.json - SHA updates in workflow lock files
- Playwright domain additions to workflow allow-lists
- Removal of 6 action pin entries in
-
Documentation changes (NOT mentioned in description):
- New Playground glossary entry
The PR description should accurately reflect all changes being made. Either:
- Split this into separate PRs (one for functional patterns, one for Playwright support)
- Update the PR description to accurately describe all changes included
| id: stale-repos | ||
| name: Run stale_repos tool | ||
| uses: github/stale-repos@a21e55567b83cf3c3f3f9085d3038dc6cee02598 # v3.0.2 | ||
| uses: github/stale-repos@3477b6488008d9411aaf22a0924ec7c1f6a69980 # v3.0.2 |
There was a problem hiding this comment.
Inconsistent version reference in comment
The SHA is being changed from a21e55567b83cf3c3f3f9085d3038dc6cee02598 to 3477b6488008d9411aaf22a0924ec7c1f6a69980, but the comment still references "v3.0.2".
Looking at the action_pins.json file, the new SHA (3477b64...) corresponds to the github/stale-repos@v3 entry (not v3.0.2). The old v3.0.2 entry with SHA a21e555... is being removed from action_pins.json.
Either:
- The comment should be updated to
# v3to match the actual pin being used, or - The SHA should use a different value if v3.0.2 is specifically needed
This mismatch could cause confusion about which version is actually being used.
| uses: github/stale-repos@3477b6488008d9411aaf22a0924ec7c1f6a69980 # v3.0.2 | |
| uses: github/stale-repos@3477b6488008d9411aaf22a0924ec7c1f6a69980 # v3 |
Apply moderate, tasteful functional/immutability improvements to the
pkg/clipackage to eliminate unnecessary mutable state and make data transformations more declarative.Round-Robin Progress: First package processed. Next run will process
pkg/cli/fileutil.Summary of Changes
1. Immutability / Functional Initialization —
compile_config.goReplaced imperative nested loop initialization in
sanitizeValidationResultswithsliceutil.Map, and extracted a pure helper functionsanitizeCompileValidationError.Before:
After:
2. Transformative Operation —
audit_report.goReplaced
var jobs []JobData+ append loop withsliceutil.Mapfor the job data building inbuildAuditData.Before:
After:
3. Transformative Operation —
audit_report_analysis.goReplaced
var failedJobs []string+ conditional append loop withsliceutil.FilterMapingenerateFailureAnalysis.Before:
After:
Benefits
sanitizeCompileValidationErroras a pure function (testable in isolation)sliceutil.Map/sliceutil.FilterMaputilities already in the codebaseTesting
TestGenerateFindings,TestGenerateRecommendations,TestGenerateFailureAnalysis,TestBuildAuditData*)go vet ./pkg/cli/cleanmake fmtcleanReferences: