-
Notifications
You must be signed in to change notification settings - Fork 28
Extract duplicate safe-output validation logic into shared helpers #4575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Create safe_output_helpers.cjs with parseAllowedItems, parseMaxCount, and resolveTarget functions - Refactor add_labels.cjs to use shared helpers (reduced by 37 lines) - Refactor add_reviewer.cjs to use shared helpers (reduced by 32 lines) - Add comprehensive tests for safe_output_helpers (34 test cases) - Update Go embedding to include new helper file - Net reduction: 69 lines of duplicate code Benefits: - Single source of truth for validation logic - Consistent error messages across safe outputs - Easier to maintain and extend - Reduced risk of divergent fixes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Agentic Changeset Generator completed successfully. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR successfully extracts duplicate safe-output validation logic from add_labels.cjs and add_reviewer.cjs into a new shared helper module safe_output_helpers.cjs. The refactoring reduces code duplication by consolidating approximately 120 lines of identical validation logic for parsing allowed items, validating max counts, and resolving target numbers.
Key changes:
- Created new shared module with three helper functions:
parseAllowedItems(),parseMaxCount(), andresolveTarget() - Refactored
add_labels.cjs(183→150 lines, -18%) andadd_reviewer.cjs(217→179 lines, -17.5%) to use the shared helpers - Added comprehensive test coverage with 34 test cases covering various scenarios
- Updated compiled workflow files (
.lock.yml) to include inlined helper functions
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_output_helpers.cjs | New shared helper module providing common validation and target resolution logic for safe-output scripts |
| pkg/workflow/js/safe_output_helpers.test.cjs | Comprehensive test suite with 34 test cases covering all three helper functions |
| pkg/workflow/js/add_labels.cjs | Refactored to use shared helpers, reducing duplicate code by 33 lines |
| pkg/workflow/js/add_labels.test.cjs | Updated test expectations to match new error message formats |
| pkg/workflow/js/add_reviewer.cjs | Refactored to use shared helpers, reducing duplicate code by 38 lines |
| pkg/workflow/js.go | Added embedding and registration of the new helper module |
| .github/workflows/poem-bot.lock.yml | Compiled workflow with inlined helper functions |
| .github/workflows/issue-classifier.lock.yml | Compiled workflow with inlined helper functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,353 @@ | |||
| import { describe, it, expect } from "vitest"; | |||
Copilot
AI
Nov 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing beforeEach import from vitest. The beforeEach function is used on line 6 but is not imported in the import statement on line 1.
| import { describe, it, expect } from "vitest"; | |
| import { describe, it, expect, beforeEach } from "vitest"; |
add_labels.cjsandadd_reviewer.cjscontained ~120 lines of identical validation logic for parsing allowed items, validating max counts, and resolving target numbers.Changes
New module
safe_output_helpers.cjswith three functions:parseAllowedItems()- Parse comma-separated allowed listsparseMaxCount()- Validate max count with structured error handlingresolveTarget()- Resolve target issue/PR numbers with proper context validationRefactored scripts to use shared helpers:
add_labels.cjs: 183 → 150 lines (-18%)add_reviewer.cjs: 217 → 179 lines (-17.5%)Error handling distinction via
shouldFailflag:core.info()core.setFailed()Usage
Net: -71 lines duplicate code, +173 lines reusable helpers, +353 lines test coverage (34 test cases).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.