Conversation
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small structural/idiomatic refactors across PR Metrics’ task implementation (GitHub + Azure Pipelines), aiming to improve robustness (URL parsing, initialization concurrency), reduce duplication, and prevent accidental state mutation.
Changes:
- Refactors initialization and helper logic (promise-lock initialization in
CodeMetrics, numeric initialization helper inInputs, numeric env-var helper inGitInvoker). - Improves safety/maintainability (defensive copies for internal arrays,
URLparsing for repo URIs, extracteduserAgentconstant, extension list dedupe,path.extname()usage). - Minor idiom cleanups (remove redundant
Promise.resolve(), build comment viajoin("")).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/task/tests/repos/gitHubReposInvoker.spec.ts | Adds coverage for non-parseable repo URI when running on Azure Pipelines. |
| src/task/src/utilities/constants.ts | Introduces a shared userAgent constant for GitHub API calls. |
| src/task/src/runners/azurePipelinesRunnerInvoker.ts | Simplifies exec() return by removing redundant Promise.resolve() in an async method. |
| src/task/src/repos/gitHubReposInvoker.ts | Uses shared userAgent; replaces manual URI splitting with new URL() parsing for Azure Pipelines initialization; removes redundant Promise.resolve(). |
| src/task/src/pullRequests/pullRequestComments.ts | Refactors comment construction to use parts.join("") instead of string concatenation. |
| src/task/src/metrics/inputsDefault.ts | Removes duplicate file extensions from default code-extension list. |
| src/task/src/metrics/inputs.ts | Deduplicates numeric input parsing/validation via initializeNumeric(). |
| src/task/src/metrics/codeMetrics.ts | Reworks initialization to a shared promise; returns defensive copies of arrays; switches extension extraction to path.extname(). |
| src/task/src/git/gitInvoker.ts | Adds getNumericEnvironmentVariable() helper to reduce repeated env-var parsing/validation logic. |
| private matchFileExtension(fileName: string): boolean { | ||
| this._logger.logDebug("* CodeMetrics.matchFileExtension()"); | ||
|
|
||
| const fileExtensionIndex: number = fileName.lastIndexOf("."); | ||
| const fileExtension: string = fileName | ||
| .substring(fileExtensionIndex + 1) | ||
| .toLowerCase(); | ||
| const fileExtension: string = path.extname(fileName).slice(1).toLowerCase(); | ||
| const result: boolean = this._inputs.codeFileExtensions.has(fileExtension); |
There was a problem hiding this comment.
path.extname() returns an empty string for dotfiles like .env, so fileExtension becomes "" and these files will never match even though InputsDefault.codeFileExtensions includes "env". This is a behavior change vs the previous lastIndexOf('.') logic and likely causes .env-style files to be incorrectly excluded from metrics. Consider special-casing dotfiles (e.g., when extname is empty and the basename starts with a single '.') or adding a fallback extraction so .env maps to env, and add/adjust a unit test to lock in the intended behavior.
Summary
_isInitializedguard with a stored promise lock (_initializePromise) to ensure concurrent callers share a single initialization and errors propagate correctly.getFilesNotRequiringReview()andgetDeletedFilesNotRequiringReview()now return shallow copies to prevent callers from mutating internal state.split("/")URL parsing ininitializeForAzureDevOps()withnew URL()for safer, more robust parsing. Adds test for non-parseable URLs.Promise.resolve()wrapping: SimplifyisAccessTokenAvailable()ingitHubReposInvoker.tsandexec()inazurePipelinesRunnerInvoker.tsby removing redundantPromise.resolve()inasyncmethods."PRMetrics/v1.7.13"user agent string intoconstants.tsas a single source of truth.fcgi,inc,h,cgi,spec) frominputsDefault.tscode file extensions list.path.extname()for extension extraction: Replace manuallastIndexOf(".")/substring()logic inmatchFileExtension()withpath.extname().getNumericEnvironmentVariable()ingitInvoker.tsto eliminate repeated get-env/check-undefined/check-numeric pattern inpullRequestIdForAzurePipelines.initializeNumeric()helper: Add a generic numeric initialisation helper ininputs.tsto deduplicateinitializeBaseSize()andinitializeGrowthRate().initializeTestFactor()retains its special-case handling for zero.getMetricsComment(): Replace string concatenation withparts.join("")for cleaner construction of the metrics comment.