Conversation
- Remove unnecessary 'item' alias (rename message parameter to 'item' directly) - Fix JSDoc @param tag to match parameter name - Add comprehensive test file with 20 test cases covering: - main factory function - close with explicit discussion_number - close from context - comment addition (with/without body) - close with reason - error handling (no number, invalid number) - max count enforcement - required labels validation - required title prefix validation - already-closed discussion handling - staged mode preview - GraphQL error handling - discussion not found handling Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Pull request created: #17824 |
There was a problem hiding this comment.
Pull request overview
This PR is part of the jsweep JavaScript cleanup effort, aiming to clean up close_discussion.cjs by removing what was perceived as an unnecessary alias and adding comprehensive test coverage.
Changes:
- Modified the
handleCloseDiscussionfunction parameter frommessagetoitemand removed theconst item = message;alias - Updated JSDoc to reflect the parameter name change
- Added a comprehensive test suite with 20 test cases covering various scenarios including edge cases, error handling, and staged mode
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/js/close_discussion.cjs | Changed function parameter from message to item, removed alias, updated JSDoc |
| actions/setup/js/close_discussion.test.cjs | Added comprehensive test suite with 20 test cases covering success paths, error cases, validation, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param {Object} item - The close_discussion message to process | ||
| * @param {Object} resolvedTemporaryIds - Map of temporary IDs to {repo, number} | ||
| * @returns {Promise<Object>} Result with success/error status | ||
| */ | ||
| return async function handleCloseDiscussion(message, resolvedTemporaryIds) { | ||
| return async function handleCloseDiscussion(item, resolvedTemporaryIds) { |
There was a problem hiding this comment.
This change breaks the established convention in the codebase. All similar handler functions use message as the parameter name and then create an alias const item = message; inside the function body. This pattern is consistently followed across the codebase in files like:
close_issue.cjs:112-async function handleCloseIssue(message, resolvedTemporaryIds)withconst item = message;at line 124close_pull_request.cjs:109-async function handleClosePullRequest(message, resolvedTemporaryIds)withconst item = message;at line 121add_comment.cjs:322-async function handleAddComment(message, resolvedTemporaryIds)withconst item = message;at line 334- And 10 other handler files following the same pattern
The parameter should be named message (not item), and the alias const item = message; should be retained for consistency with the rest of the codebase.
See below for a potential fix:
* @param {Object} message - The close_discussion message to process
* @param {Object} resolvedTemporaryIds - Map of temporary IDs to {repo, number}
* @returns {Promise<Object>} Result with success/error status
*/
return async function handleCloseDiscussion(message, resolvedTemporaryIds) {
const item = message;
Summary
Cleaned
actions/setup/js/close_discussion.cjsas part of the jsweep JavaScript unbloating effort.Context Type
github-script context — uses
core.info(),core.warning(),core.error(),github.graphql(), andcontextglobals.Changes Made
close_discussion.cjsitemalias: ThehandleCloseDiscussionfunction parameter was renamed frommessagetoitemdirectly, eliminating the redundantconst item = message;alias on line 195.@paramtag: Updated the@paramtag name to match the actual parameter name (iteminstead ofmessage), fixing a TypeScript type checking error.close_discussion.test.cjs(new file)Created a comprehensive test suite with 20 test cases covering:
Test scenarios:
discussion_numberRESOLVED)discussion_numberis invalid (non-numeric)Test Improvements
Validation Checks
All four validation checks passed:
npm run format:cjs✓npm run lint:cjs✓npm run typecheck✓npm run test:js -- --no-file-parallelism✓ (20/20 close_discussion tests passing)Warning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org