From 5813113354f2c31be07162faf8783626161b4181 Mon Sep 17 00:00:00 2001 From: Sam Lin Date: Fri, 26 Jun 2026 23:04:37 -0500 Subject: [PATCH] refactor: simplify report/format helpers (/simplify pass) Quality cleanups from a 4-angle review of the composite-action diff: - report-common.mjs: add buildReportContext(context); both report wrappers (format-report, jscpd-report) now call it instead of hand-building the identical owner/repo/sha/cwd/runId/commit object. - jscpd-report.mjs: buildJscpdMessage takes the pre-computed `header` (like generateReport) instead of recomputing getReportHeader internally; wrapper passes it. - format-args.mjs: collapse buildDefaultOptions' 4 near-identical blocks to a base + spread (only the two real overrides remain); simplify checkIsDryRun's granular check via a `dry()` helper; drop the unused `config` field from planFormat's return; make internal-only buildArgs/FormatType module-private. - comment.mjs: run the independent listComments + getAuthenticated calls with Promise.all (getAuthenticated still degrades to undefined on 403). - action.yml: write the push stderr log to $RUNNER_TEMP instead of the repo tree, and drop the duplicate ::error:: annotation on exec failure (per-command one remains). Skipped (documented as follow-ups): unifying the jscpd config resolution that happens in both the shell runner and the JS threshold read (structural; risky without an e2e run), and the github-script import boilerplate (explicit per-step is clearer). biome check + 33 node:test green; wrappers smoke-tested; bash -n + YAML clean. Claude-Session: https://claude.ai/code/session_01JxEe85CyGm5rCeoLFdUZed --- __tests__/jscpd-report.test.mjs | 3 +- action.yml | 12 ++++--- scripts/format-args.mjs | 58 ++++++++++++--------------------- scripts/jscpd-report.mjs | 9 ++--- scripts/report-common.mjs | 18 ++++++++++ scripts/steps/comment.mjs | 25 +++++++------- scripts/steps/format-report.mjs | 11 ++----- scripts/steps/jscpd-report.mjs | 12 ++----- 8 files changed, 70 insertions(+), 78 deletions(-) diff --git a/__tests__/jscpd-report.test.mjs b/__tests__/jscpd-report.test.mjs index dda1042..aee5527 100644 --- a/__tests__/jscpd-report.test.mjs +++ b/__tests__/jscpd-report.test.mjs @@ -34,7 +34,8 @@ describe('isOverThreshold', () => { describe('buildJscpdMessage', () => { it('strips the default header, adds links and footer', () => { - const md = buildJscpdMessage('# Copy/paste detection report\n\nsome table', duplicates, '/repo/src', '/repo/src', ctx); + const header = getReportHeader('/repo/src'); + const md = buildJscpdMessage('# Copy/paste detection report\n\nsome table', duplicates, header, '/repo/src', ctx); assert.match(md, /## ❌ DUPLICATED CODE FOUND - \/repo\/src/); assert.doesNotMatch(md, /# Copy\/paste detection report/); assert.match(md, / JSCPD Details <\/summary>/); diff --git a/action.yml b/action.yml index be6c1b6..2601d6b 100644 --- a/action.yml +++ b/action.yml @@ -213,9 +213,10 @@ runs: echo "formatResult=$format_result" >> "$GITHUB_OUTPUT" # Infrastructure/runner failures fail unconditionally; formatting findings - # remain gated by the failFast step. + # remain gated by the failFast step. The per-command ::error:: above already + # annotated the failure, so just exit here. if [ "$exec_failed" = "true" ]; then - echo "::error title=DOTNET FORMAT Check::dotnet format failed to execute" + echo "dotnet format failed to execute (see error above)" exit 1 fi @@ -311,11 +312,12 @@ runs: attempt=1 while [ "$attempt" -le 3 ]; do echo "Pushing changes… (attempt $attempt of 3)" + ERR_LOG="$RUNNER_TEMP/df-push-err.log" set +e - push_out=$(git push --porcelain origin "$BRANCH:$BRANCH" 2>push_err.log) + push_out=$(git push --porcelain origin "$BRANCH:$BRANCH" 2>"$ERR_LOG") set -e - push_err=$(cat push_err.log 2>/dev/null || true) - rm -f push_err.log + push_err=$(cat "$ERR_LOG" 2>/dev/null || true) + rm -f "$ERR_LOG" if printf '%s' "$push_out" | grep -qE '\[rejected\]|\[remote rejected\]'; then echo "Updates were rejected; fetching and rebasing onto $BRANCH" git fetch diff --git a/scripts/format-args.mjs b/scripts/format-args.mjs index 1c37560..26b5e85 100644 --- a/scripts/format-args.mjs +++ b/scripts/format-args.mjs @@ -10,7 +10,7 @@ import process from 'node:process'; /** Mirrors REPORT_PATH from src/common.ts (computed from cwd at load time). */ export const DEFAULT_REPORT_DIR = `${process.cwd()}/.dotnet-format`; -export const FormatType = { +const FormatType = { all: 'all', style: 'style', analyzers: 'analyzers', @@ -46,7 +46,7 @@ function resolveEnabled(block, defaultValue) { * @param {string} type * @returns {string[]} */ -export function buildArgs(options, onlyChangedFiles, changedFiles, type) { +function buildArgs(options, onlyChangedFiles, changedFiles, type) { /** @type {string[]} */ const args = []; if (options.verifyNoChanges) { @@ -120,37 +120,22 @@ export function generateFormatCommandArgs(config, workspace, changedFiles = [], * @returns {Record} */ export function buildDefaultOptions(inputs) { - const verifyNoChanges = inputs.action === 'check'; + // Shared per-block defaults; the blocks differ only in the two overrides below. + const base = { + verifyNoChanges: inputs.action === 'check', + severity: inputs.severityLevel, + verbosity: inputs.logLevel, + noRestore: !!inputs.nugetConfigPath + }; return { nugetConfigPath: inputs.nugetConfigPath, projectFileName: inputs.projectFileName, onlyChangedFiles: inputs.onlyChangedFiles, - options: { - verifyNoChanges, - severity: inputs.severityLevel, - verbosity: inputs.logLevel, - noRestore: !!inputs.nugetConfigPath - }, - whitespaceOptions: { - verifyNoChanges, - folder: true, - severity: inputs.severityLevel, - verbosity: inputs.logLevel, - noRestore: !!inputs.nugetConfigPath - }, - analyzersOptions: { - verifyNoChanges, - severity: inputs.severityLevel, - verbosity: inputs.logLevel, - noRestore: !!inputs.nugetConfigPath - }, - styleOptions: { - verifyNoChanges, - severity: inputs.severityLevel, - verbosity: inputs.logLevel, - // Quirk preserved from the original: style uses dotnetFormatConfigPath here. - noRestore: !!inputs.dotnetFormatConfigPath - } + options: { ...base }, + whitespaceOptions: { ...base, folder: true }, + analyzersOptions: { ...base }, + // Quirk preserved from the original: style keys noRestore off dotnetFormatConfigPath. + styleOptions: { ...base, noRestore: !!inputs.dotnetFormatConfigPath } }; } @@ -178,25 +163,22 @@ export function checkIsDryRun(config) { if (isEnabledBlock(config.options, false)) { return config.options?.verifyNoChanges ?? false; } - const wEnabled = isEnabledBlock(config.whitespaceOptions, false); - const aEnabled = isEnabledBlock(config.analyzersOptions, false); - const sEnabled = isEnabledBlock(config.styleOptions, false); - const w = (wEnabled && !!config.whitespaceOptions?.verifyNoChanges) || !wEnabled; - const a = (aEnabled && !!config.analyzersOptions?.verifyNoChanges) || !aEnabled; - const s = (sEnabled && !!config.styleOptions?.verifyNoChanges) || !sEnabled; - return w && a && s; + // A granular block is "dry" when it is disabled, or enabled and verifying no + // changes — i.e. `(enabled && verify) || !enabled` simplifies to `!enabled || verify`. + const dry = block => !isEnabledBlock(block, false) || !!block?.verifyNoChanges; + return dry(config.whitespaceOptions) && dry(config.analyzersOptions) && dry(config.styleOptions); } /** * Convenience: finalize a merged config and emit everything the runner needs. * @param {Record} mergedConfig * @param {{ workspace: string, changedFiles?: string[], isOnlyChangedFiles?: boolean, reportDir?: string }} opts - * @returns {{ config: Record, commands: string[][], isDryRun: boolean }} + * @returns {{ commands: string[][], isDryRun: boolean }} */ export function planFormat(mergedConfig, opts) { const { workspace, changedFiles = [], isOnlyChangedFiles = false, reportDir = DEFAULT_REPORT_DIR } = opts; const config = finalizeEnabled(mergedConfig); const commands = generateFormatCommandArgs(config, workspace, changedFiles, { isOnlyChangedFiles, reportDir }); const isDryRun = checkIsDryRun(config); - return { config, commands, isDryRun }; + return { commands, isDryRun }; } diff --git a/scripts/jscpd-report.mjs b/scripts/jscpd-report.mjs index f4d3cf3..9edf472 100644 --- a/scripts/jscpd-report.mjs +++ b/scripts/jscpd-report.mjs @@ -49,15 +49,17 @@ export function isOverThreshold(report, threshold) { /** * Build the PR comment / summary message from the jscpd markdown report content - * and the parsed duplicates. Mirrors the message built in postReport. + * and the parsed duplicates. Mirrors the message built in postReport. Takes the + * pre-computed `header` (like `generateReport` in dotnet-report.mjs) so the caller + * does not compute it twice. * @param {string} markdownReportContent Raw contents of jscpd's markdown report. * @param {any[]} duplicates - * @param {string} workspace + * @param {string} header * @param {string} scanPath * @param {import('./report-common.mjs').ReportContext} ctx * @returns {string} */ -export function buildJscpdMessage(markdownReportContent, duplicates, workspace, scanPath, ctx) { +export function buildJscpdMessage(markdownReportContent, duplicates, header, scanPath, ctx) { const report = markdownReportContent.replace('# Copy/paste detection report', ''); let markdown = '
\n'; markdown += ` JSCPD Details \n\n`; @@ -68,7 +70,6 @@ export function buildJscpdMessage(markdownReportContent, duplicates, workspace, markdown += '\n'; } markdown += '
\n'; - const header = getReportHeader(workspace); return `${header} \n\n${report}\n\n ${markdown}\n\n ${getReportFooter(ctx)}`; } diff --git a/scripts/report-common.mjs b/scripts/report-common.mjs index dcee5f3..f806f89 100644 --- a/scripts/report-common.mjs +++ b/scripts/report-common.mjs @@ -12,6 +12,24 @@ * @property {string} [commit] Commit to credit in the footer (PR head sha or sha). */ +/** + * Build the {@link ReportContext} from github-script's `context` (owner/repo/sha + + * the PR-head-or-sha commit to credit). Shared by the dotnet format and jscpd report + * steps so the context shape lives in one place. + * @param {any} context + * @returns {ReportContext} + */ +export function buildReportContext(context) { + return { + owner: context.repo.owner, + repo: context.repo.repo, + sha: context.sha, + cwd: process.cwd(), + runId: context.runId, + commit: context.payload?.pull_request?.head?.sha || context.sha + }; +} + /** * @param {ReportContext} ctx * @returns {string} diff --git a/scripts/steps/comment.mjs b/scripts/steps/comment.mjs index eb6fd82..f4c02f0 100644 --- a/scripts/steps/comment.mjs +++ b/scripts/steps/comment.mjs @@ -10,18 +10,19 @@ * @returns {Promise} */ export async function getExistingCommentId(github, context, header) { - const comments = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number - }); - let userLogin; - try { - userLogin = (await github.rest.users.getAuthenticated()).data?.login; - } catch { - // token without user scope; fall back to matching the Bot user type - userLogin = undefined; - } + // The two calls are independent; run them together. getAuthenticated may 403 on a + // token without user scope — fall back to matching the Bot user type in that case. + const [comments, userLogin] = await Promise.all([ + github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number + }), + github.rest.users + .getAuthenticated() + .then(r => r.data?.login) + .catch(() => undefined) + ]); const existing = comments.data?.find(c => { const isBotUserType = c.user?.type === 'Bot' || c.user?.login === userLogin; return isBotUserType && c.body?.startsWith(header); diff --git a/scripts/steps/format-report.mjs b/scripts/steps/format-report.mjs index 41880f9..90c55b7 100644 --- a/scripts/steps/format-report.mjs +++ b/scripts/steps/format-report.mjs @@ -3,6 +3,7 @@ // github-script's { github, context, core }; report dir + inputs arrive via env. import { generateReport, getReportFiles, getReportHeader } from '../dotnet-report.mjs'; +import { buildReportContext } from '../report-common.mjs'; import { upsertComment } from './comment.mjs'; /** @@ -21,15 +22,7 @@ export async function run({ github, context, core }) { } const header = getReportHeader(workspace); - const reportCtx = { - owner: context.repo.owner, - repo: context.repo.repo, - sha: context.sha, - cwd: process.cwd(), - runId: context.runId, - commit: context.payload?.pull_request?.head?.sha || context.sha - }; - const message = generateReport(reportFiles, header, reportCtx); + const message = generateReport(reportFiles, header, buildReportContext(context)); if (!message) { return; } diff --git a/scripts/steps/jscpd-report.mjs b/scripts/steps/jscpd-report.mjs index 74dce86..e2e463a 100644 --- a/scripts/steps/jscpd-report.mjs +++ b/scripts/steps/jscpd-report.mjs @@ -8,6 +8,7 @@ import * as fs from 'node:fs'; import path from 'node:path'; import { buildAnnotations, buildJscpdMessage, getReportHeader, isOverThreshold } from '../jscpd-report.mjs'; import { resolveConfig } from '../read-config.mjs'; +import { buildReportContext } from '../report-common.mjs'; import { upsertComment } from './comment.mjs'; import { makeLoader } from './load-config.mjs'; @@ -46,21 +47,14 @@ export async function run({ github, context, core, exec }) { const threshold = cfg.threshold ?? 0; const cwd = process.cwd(); - const reportCtx = { - owner: context.repo.owner, - repo: context.repo.repo, - sha: context.sha, - cwd, - runId: context.runId, - commit: context.payload?.pull_request?.head?.sha || context.sha - }; + const reportCtx = buildReportContext(context); const header = getReportHeader(workspace); // Build the message from jscpd's markdown report, post it, and overwrite the // markdown file with the full message (so the uploaded artifact matches). const mdFile = findByExt(outputDir, '.md'); const mdContent = mdFile ? fs.readFileSync(mdFile, 'utf8') : ''; - const message = buildJscpdMessage(mdContent, duplicates, workspace, scanPath, reportCtx); + const message = buildJscpdMessage(mdContent, duplicates, header, scanPath, reportCtx); if (mdFile) { fs.writeFileSync(mdFile, message); }