-
Notifications
You must be signed in to change notification settings - Fork 5
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
mops watch #254
mops watch #254
Changes from 9 commits
97aeb43
63a5628
27184b6
ee2252d
20dfb99
1761cfa
713db3f
5842ec5
80f0757
63c34ba
7300fda
e44e856
a4c45cd
64821d3
d3a27a0
7fd4974
566e00c
8a2e9bf
5e34b3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,14 +5,25 @@ import {Reporter} from './reporter.js'; | |||||
import {TestMode} from '../../../types.js'; | ||||||
|
||||||
export class SilentReporter implements Reporter { | ||||||
total = 0; | ||||||
passed = 0; | ||||||
failed = 0; | ||||||
skipped = 0; | ||||||
passedFiles = 0; | ||||||
failedFiles = 0; | ||||||
passedNamesFlat : string[] = []; | ||||||
flushOnError = true; | ||||||
errorOutput = ''; | ||||||
onProgress = () => {}; | ||||||
|
||||||
addFiles(_files : string[]) {} | ||||||
constructor(flushOnError = true, onProgress = () => {}) { | ||||||
this.flushOnError = flushOnError; | ||||||
this.onProgress = onProgress; | ||||||
} | ||||||
|
||||||
addFiles(files : string[]) { | ||||||
this.total = files.length; | ||||||
} | ||||||
|
||||||
addRun(file : string, mmf : MMF1, state : Promise<void>, _mode : TestMode) { | ||||||
state.then(() => { | ||||||
|
@@ -30,10 +41,17 @@ export class SilentReporter implements Reporter { | |||||
this.failedFiles += Number(mmf.failed !== 0); | ||||||
|
||||||
if (mmf.failed) { | ||||||
console.log(chalk.red('✖'), absToRel(file)); | ||||||
mmf.flush('fail'); | ||||||
console.log('-'.repeat(50)); | ||||||
let output = `${chalk.red('✖'), absToRel(file)}\n${mmf.getErrorMessages().join('\n')}\n${'-'.repeat(50)}`; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix Incorrect Use of Comma Operator in Template Literal At line 44, the use of the comma operator within the template literal results in only the last expression being included. This means Suggested Fix: Replace the comma with a -let output = `${chalk.red('✖'), absToRel(file)}\n${mmf.getErrorMessages().join('\n')}\n${'-'.repeat(50)}`;
+let output = `${chalk.red('✖')} ${absToRel(file)}\n${mmf.getErrorMessages().join('\n')}\n${'-'.repeat(50)}`; 📝 Committable suggestion
Suggested change
|
||||||
|
||||||
if (this.flushOnError) { | ||||||
console.log(output); | ||||||
} | ||||||
else { | ||||||
this.errorOutput = `${this.errorOutput}\n${output}`.trim(); | ||||||
} | ||||||
Comment on lines
+46
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor Error Output Accumulation for Efficiency Accumulating error messages using string concatenation may become inefficient with a large number of errors. Consider using an array to collect error messages and join them when needed. Apply this refactor to improve performance and maintainability: - else {
- this.errorOutput = `${this.errorOutput}\n${output}`.trim();
- }
+ else {
+ if (!this.errorOutputArray) {
+ this.errorOutputArray = [];
+ }
+ this.errorOutputArray.push(output);
+ } Then, when you need to access the accumulated error output: getErrorOutput() {
return this.errorOutputArray.join('\n');
} |
||||||
} | ||||||
|
||||||
this.onProgress(); | ||||||
}); | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -127,7 +127,7 @@ async function runAll(reporterName : ReporterName | undefined, filter = '', mode | |
return done; | ||
} | ||
|
||
export async function testWithReporter(reporterName : ReporterName | Reporter | undefined, filter = '', defaultMode : TestMode = 'interpreter', replicaType : ReplicaName, watch = false) : Promise<boolean> { | ||
export async function testWithReporter(reporterName : ReporterName | Reporter | undefined, filter = '', defaultMode : TestMode = 'interpreter', replicaType : ReplicaName, watch = false, signal ?: AbortSignal) : Promise<boolean> { | ||
let rootDir = getRootDir(); | ||
let files : string[] = []; | ||
let libFiles = globSync('**/test?(s)/lib.mo', globConfig); | ||
|
@@ -274,6 +274,10 @@ export async function testWithReporter(reporterName : ReporterName | Reporter | | |
|
||
await startReplicaOnce(replica, replicaType); | ||
|
||
if (signal?.aborted) { | ||
return; | ||
} | ||
Comment on lines
+320
to
+322
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure child processes are terminated when While checking To address this, consider adding an event listener to the // Example for handling buildProc in Wasi mode
let buildProc = spawn(mocPath, [`-o=${wasmFile}`, '-wasi-system-api', ...mocArgs]);
+ if (signal) {
+ signal.addEventListener('abort', () => {
+ buildProc.kill();
+ });
+ }
pipeMMF(buildProc, mmf).then(async () => {
// existing code... Repeat similar logic for other child processes like Also applies to: 289-291, 309-311, 330-332 |
||
|
||
let canisterName = path.parse(file).name; | ||
let idlFactory = ({IDL} : any) => { | ||
return IDL.Service({'runTests': IDL.Func([], [], [])}); | ||
|
@@ -282,6 +286,10 @@ export async function testWithReporter(reporterName : ReporterName | Reporter | | |
|
||
let {stream} = await replica.deploy(canisterName, wasmFile, idlFactory); | ||
|
||
if (signal?.aborted) { | ||
return; | ||
} | ||
|
||
pipeStdoutToMMF(stream, mmf); | ||
|
||
let actor = await replica.getActor(canisterName) as _SERVICE; | ||
|
@@ -298,6 +306,10 @@ export async function testWithReporter(reporterName : ReporterName | Reporter | | |
}); | ||
} | ||
|
||
if (signal?.aborted) { | ||
return; | ||
} | ||
|
||
globalThis.mopsReplicaTestRunning = true; | ||
await actor.runTests(); | ||
globalThis.mopsReplicaTestRunning = false; | ||
|
@@ -315,6 +327,10 @@ export async function testWithReporter(reporterName : ReporterName | Reporter | | |
} | ||
}); | ||
|
||
if (signal?.aborted) { | ||
return; | ||
} | ||
|
||
reporter.addRun(file, mmf, promise, mode); | ||
|
||
await promise; | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,155 @@ | ||||||||||||||||||||||||||
import chalk from 'chalk'; | ||||||||||||||||||||||||||
import os from 'node:os'; | ||||||||||||||||||||||||||
import {promisify} from 'node:util'; | ||||||||||||||||||||||||||
import {exec, execSync} from 'node:child_process'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
import {ErrorChecker} from './error-checker.js'; | ||||||||||||||||||||||||||
import {Generator} from './generator.js'; | ||||||||||||||||||||||||||
import {parallel} from '../../parallel.js'; | ||||||||||||||||||||||||||
import {getRootDir} from '../../mops.js'; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
export class Deployer { | ||||||||||||||||||||||||||
verbose = false; | ||||||||||||||||||||||||||
canisters : Record<string, string> = {}; | ||||||||||||||||||||||||||
status : 'pending' | 'running' | 'syntax-error' | 'dfx-error' | 'error' | 'success' = 'pending'; | ||||||||||||||||||||||||||
errorChecker : ErrorChecker; | ||||||||||||||||||||||||||
generator : Generator; | ||||||||||||||||||||||||||
success = 0; | ||||||||||||||||||||||||||
errors : string[] = []; | ||||||||||||||||||||||||||
aborted = false; | ||||||||||||||||||||||||||
controllers = new Map<string, AbortController>(); | ||||||||||||||||||||||||||
currentRun : Promise<any> | undefined; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
constructor({verbose, canisters, errorChecker, generator} : {verbose : boolean, canisters : Record<string, string>, errorChecker : ErrorChecker, generator : Generator}) { | ||||||||||||||||||||||||||
this.verbose = verbose; | ||||||||||||||||||||||||||
this.canisters = canisters; | ||||||||||||||||||||||||||
this.errorChecker = errorChecker; | ||||||||||||||||||||||||||
this.generator = generator; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
reset() { | ||||||||||||||||||||||||||
this.status = 'pending'; | ||||||||||||||||||||||||||
this.success = 0; | ||||||||||||||||||||||||||
this.errors = []; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
async abortCurrent() { | ||||||||||||||||||||||||||
this.aborted = true; | ||||||||||||||||||||||||||
for (let controller of this.controllers.values()) { | ||||||||||||||||||||||||||
controller.abort(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
this.controllers.clear(); | ||||||||||||||||||||||||||
await this.currentRun; | ||||||||||||||||||||||||||
this.reset(); | ||||||||||||||||||||||||||
this.aborted = false; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
async run(onProgress : () => void) { | ||||||||||||||||||||||||||
await this.abortCurrent(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (this.errorChecker.status === 'error') { | ||||||||||||||||||||||||||
this.status = 'syntax-error'; | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (Object.keys(this.canisters).length === 0) { | ||||||||||||||||||||||||||
this.status = 'success'; | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
let rootDir = getRootDir(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||
execSync('dfx ping', {cwd: rootDir}); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
catch (error) { | ||||||||||||||||||||||||||
this.status = 'dfx-error'; | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
Comment on lines
+47
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using an asynchronous approach for the DFX check. The initial checks and verifications in the Example: try {
await promisify(execFile)('dfx', ['ping'], { cwd: rootDir });
} catch (error) {
this.status = 'dfx-error';
onProgress();
return;
}
Comment on lines
+64
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using an asynchronous approach for the DFX check. The current implementation uses Example: try {
await promisify(execFile)('dfx', ['ping'], { cwd: rootDir });
} catch (error) {
this.status = 'dfx-error';
onProgress();
return;
} |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
this.status = 'running'; | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// create canisters (sequentially to avoid DFX errors) | ||||||||||||||||||||||||||
let resolve : (() => void) | undefined; | ||||||||||||||||||||||||||
this.currentRun = new Promise<void>((res) => resolve = res); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid assignment within expressions for improved clarity. Assigning a variable within an expression can make the code less readable and harder to maintain. It's recommended to perform assignments outside of expressions. Apply this diff to make the assignment explicit: - this.currentRun = new Promise<void>((res) => resolve = res);
+ this.currentRun = new Promise<void>((res) => {
+ resolve = res;
+ }); 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome
Comment on lines
+76
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid assignment within expressions for improved clarity. The assignment within the Promise constructor can make the code less readable. Consider separating the assignment for better clarity. Apply this diff to make the assignment explicit: - this.currentRun = new Promise<void>((res) => resolve = res);
+ this.currentRun = new Promise<void>((res) => {
+ resolve = res;
+ }); 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Biome
|
||||||||||||||||||||||||||
for (let canister of Object.keys(this.canisters)) { | ||||||||||||||||||||||||||
let controller = new AbortController(); | ||||||||||||||||||||||||||
let {signal} = controller; | ||||||||||||||||||||||||||
this.controllers.set(canister, controller); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
await promisify(exec)(`dfx canister create ${canister}`, {cwd: rootDir, signal}).catch((error) => { | ||||||||||||||||||||||||||
if (error.code === 'ABORT_ERR') { | ||||||||||||||||||||||||||
return {stderr: ''}; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sanitize shell command inputs to prevent command injection vulnerabilities. Interpolating variables directly into shell commands can expose the application to command injection attacks if the variables contain malicious input. Ensure that the Consider using For the - await promisify(exec)(`dfx canister create ${canister}`, {cwd: rootDir, signal}).catch((error) => {
+ await promisify(execFile)('dfx', ['canister', 'create', canister], {cwd: rootDir, signal}).catch((error) => { For the - await promisify(exec)(`dfx build ${canister}`, {cwd: rootDir, signal}).catch((error) => {
+ await promisify(execFile)('dfx', ['build', canister], {cwd: rootDir, signal}).catch((error) => { For the - await promisify(exec)(`dfx canister install --mode=auto ${canister}`, {cwd: rootDir, signal}).catch((error) => {
+ await promisify(execFile)('dfx', ['canister', 'install', '--mode=auto', canister], {cwd: rootDir, signal}).catch((error) => { Also applies to: 107-113, 116-121 |
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
this.controllers.delete(canister); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
resolve?.(); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (this.aborted) { | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
this.currentRun = parallel(os.cpus().length, [...Object.keys(this.canisters)], async (canister) => { | ||||||||||||||||||||||||||
let controller = new AbortController(); | ||||||||||||||||||||||||||
let {signal} = controller; | ||||||||||||||||||||||||||
this.controllers.set(canister, controller); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// build | ||||||||||||||||||||||||||
if (this.generator.status !== 'success') { | ||||||||||||||||||||||||||
await promisify(exec)(`dfx build ${canister}`, {cwd: rootDir, signal}).catch((error) => { | ||||||||||||||||||||||||||
if (error.code === 'ABORT_ERR') { | ||||||||||||||||||||||||||
return {stderr: ''}; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// install | ||||||||||||||||||||||||||
await promisify(exec)(`dfx canister install --mode=auto ${canister}`, {cwd: rootDir, signal}).catch((error) => { | ||||||||||||||||||||||||||
if (error.code === 'ABORT_ERR') { | ||||||||||||||||||||||||||
return {stderr: ''}; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
throw error; | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
this.success += 1; | ||||||||||||||||||||||||||
this.controllers.delete(canister); | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
await this.currentRun; | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (!this.aborted) { | ||||||||||||||||||||||||||
this.status = 'success'; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
onProgress(); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
getOutput() : string { | ||||||||||||||||||||||||||
let get = (v : number) => v.toString(); | ||||||||||||||||||||||||||
let count = (this.status === 'running' ? get : chalk.bold[this.errors.length > 0 ? 'redBright' : 'green'])(this.errors.length || this.success); | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
if (this.status === 'pending') { | ||||||||||||||||||||||||||
return `Deploy: ${chalk.gray('(pending)')}`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (this.status === 'running') { | ||||||||||||||||||||||||||
return `Deploy: ${count}/${Object.keys(this.canisters).length} ${chalk.gray('(running)')}`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (this.status === 'syntax-error') { | ||||||||||||||||||||||||||
return `Deploy: ${chalk.gray('(errors)')}`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
if (this.status === 'dfx-error') { | ||||||||||||||||||||||||||
return `Deploy: ${chalk.gray('(dfx not running)')}`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
return `Deploy: ${count}`; | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,87 @@ | ||||||||||
import {exec} from 'node:child_process'; | ||||||||||
import {promisify} from 'node:util'; | ||||||||||
import os from 'node:os'; | ||||||||||
import chalk from 'chalk'; | ||||||||||
|
||||||||||
import {getMocPath} from '../../helpers/get-moc-path.js'; | ||||||||||
import {getRootDir} from '../../mops.js'; | ||||||||||
import {sources} from '../sources.js'; | ||||||||||
import {parallel} from '../../parallel.js'; | ||||||||||
import {globMoFiles} from './globMoFiles.js'; | ||||||||||
|
||||||||||
export class ErrorChecker { | ||||||||||
verbose = false; | ||||||||||
canisters : Record<string, string> = {}; | ||||||||||
status : 'pending' | 'running' | 'error' | 'success' = 'pending'; | ||||||||||
errors : string[] = []; | ||||||||||
|
||||||||||
constructor({verbose, canisters} : {verbose : boolean, canisters : Record<string, string>}) { | ||||||||||
this.verbose = verbose; | ||||||||||
this.canisters = canisters; | ||||||||||
} | ||||||||||
|
||||||||||
reset() { | ||||||||||
this.status = 'pending'; | ||||||||||
this.errors = []; | ||||||||||
} | ||||||||||
|
||||||||||
async run(onProgress : () => void) { | ||||||||||
this.reset(); | ||||||||||
|
||||||||||
this.status = 'running'; | ||||||||||
onProgress(); | ||||||||||
|
||||||||||
let rootDir = getRootDir(); | ||||||||||
let mocPath = getMocPath(); | ||||||||||
let deps = await sources({cwd: rootDir}); | ||||||||||
|
||||||||||
let paths = [...Object.values(this.canisters)]; | ||||||||||
if (!paths.length) { | ||||||||||
paths = globMoFiles(rootDir); | ||||||||||
} | ||||||||||
|
||||||||||
await parallel(os.cpus().length, paths, async (file) => { | ||||||||||
try { | ||||||||||
await promisify(exec)(`${mocPath} --check ${deps.join(' ')} ${file}`, {cwd: rootDir}); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix potential command injection risk when using Using Apply this diff to implement the change: - import {exec} from 'node:child_process';
+ import {execFile} from 'node:child_process';
...
- await promisify(exec)(`${mocPath} --check ${deps.join(' ')} ${file}`, {cwd: rootDir});
+ await promisify(execFile)(mocPath, ['--check', ...deps, file], {cwd: rootDir});
|
||||||||||
} | ||||||||||
catch (error : any) { | ||||||||||
error.message.split('\n').forEach((line : string) => { | ||||||||||
if (line.match(/: \w+ error \[/)) { | ||||||||||
// better formatting | ||||||||||
let str = line | ||||||||||
.replace(/: (\w+ error) \[/, (_, m1) => `: ${chalk.red(m1)} [`) | ||||||||||
.replace(/unbound type (\w+)/, `unbound type ${chalk.bold('$1')}`) | ||||||||||
.replace(/unbound variable (\w+)/, `unbound variable ${chalk.bold('$1')}`) | ||||||||||
Comment on lines
+53
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix incorrect usage of captured groups in In the Apply this diff to fix the issue: - .replace(/unbound type (\w+)/, `unbound type ${chalk.bold('$1')}`)
- .replace(/unbound variable (\w+)/, `unbound variable ${chalk.bold('$1')}`)
+ .replace(/unbound type (\w+)/, (_, m1) => `unbound type ${chalk.bold(m1)}`)
+ .replace(/unbound variable (\w+)/, (_, m1) => `unbound variable ${chalk.bold(m1)}`) 📝 Committable suggestion
Suggested change
|
||||||||||
.trim(); | ||||||||||
this.errors.push(str); | ||||||||||
} | ||||||||||
else if (line.startsWith(' ') && this.errors.length) { | ||||||||||
this.errors[this.errors.length - 1] += '\n ' + line; | ||||||||||
} | ||||||||||
else { | ||||||||||
// console.log('UNKNOWN ERROR', line); | ||||||||||
} | ||||||||||
Comment on lines
+62
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Handle unknown error lines or remove commented code for cleanliness. The else block contains commented-out code, which can clutter the codebase. It's advisable to either handle the unknown error lines appropriately or remove the commented code to maintain code clarity. Consider updating the code as follows: - // console.log('UNKNOWN ERROR', line);
+ // Optionally handle or log unknown error lines here Or remove the commented code entirely if it's not needed.
|
||||||||||
}); | ||||||||||
onProgress(); | ||||||||||
} | ||||||||||
}); | ||||||||||
|
||||||||||
this.status = this.errors.length ? 'error' : 'success'; | ||||||||||
onProgress(); | ||||||||||
} | ||||||||||
|
||||||||||
getOutput() : string { | ||||||||||
if (this.status === 'pending') { | ||||||||||
return `Errors: ${chalk.gray('(pending)')}`; | ||||||||||
} | ||||||||||
if (this.status === 'running') { | ||||||||||
return `Errors: ${chalk.gray('(running)')}`; | ||||||||||
} | ||||||||||
let output = ''; | ||||||||||
output += `Errors: ${chalk.bold[this.errors.length ? 'redBright' : 'green'](this.errors.length)}`; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance readability of dynamic chalk styling in the output. The current dynamic access of chalk styles can be made more readable. Assigning the style to a variable or using a more explicit approach improves clarity. Consider refactoring the code: - output += `Errors: ${chalk.bold[this.errors.length ? 'redBright' : 'green'](this.errors.length)}`;
+ const color = this.errors.length ? chalk.bold.redBright : chalk.bold.green;
+ output += `Errors: ${color(this.errors.length)}`; This change enhances readability by clearly showing which color function is being used based on the condition. 📝 Committable suggestion
Suggested change
|
||||||||||
if (this.verbose && this.errors.length) { | ||||||||||
output += `\n ${this.errors.join('\n ')}`; | ||||||||||
} | ||||||||||
return output; | ||||||||||
} | ||||||||||
} |
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.
🛠️ Refactor suggestion
Refactor to Avoid Redundant Default Values
The properties
flushOnError
andonProgress
are assigned default values both in their declarations and in the constructor parameters. This redundancy can be eliminated to improve code clarity.Suggested Refactor:
Option 1—Remove default values from property declarations:
Option 2—Keep defaults in property declarations and adjust constructor parameters:
Also applies to: 19-22