-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add option quiet to hide warnings in output #629
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
base: main
Are you sure you want to change the base?
Conversation
had some trouble with |
Thanks a lot for your contribution @marianfoo. We will take care of this PR in backlog item CPOUI5FOUNDATION-837. |
fixableWarningCount: 0, | ||
})); | ||
}; | ||
|
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.
Why not overwrite res
here once so it affects all the cases below?
@@ -28,7 +28,7 @@ export class Json { | |||
filePath: oLintedFile.filePath, | |||
messages: aFileMessages, | |||
errorCount: oLintedFile.errorCount, | |||
warningCount: oLintedFile.warningCount, | |||
warningCount: quiet ? 0 : oLintedFile.warningCount, |
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.
Is this necessary? warningCount
should be set to 0
in the filter function?
if (quiet) { | ||
summary += | ||
`> ${totalErrorCount} ${totalErrorCount === 1 ? "problem" : "problems"} ` + | ||
`(${totalErrorCount} ${totalErrorCount === 1 ? "error" : "errors"}) \n`; | ||
} else { | ||
summary += | ||
`> ${totalErrorCount + totalWarningCount} problems ` + | ||
`(${totalErrorCount} errors, ${totalWarningCount} warnings) \n`; | ||
} |
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.
Ideally both outputs behave the same. So If we make a distinction for singular/plural in quite mode, I would expect the same in regular mode.
Maybe something like this?
if (quiet) { | |
summary += | |
`> ${totalErrorCount} ${totalErrorCount === 1 ? "problem" : "problems"} ` + | |
`(${totalErrorCount} ${totalErrorCount === 1 ? "error" : "errors"}) \n`; | |
} else { | |
summary += | |
`> ${totalErrorCount + totalWarningCount} problems ` + | |
`(${totalErrorCount} errors, ${totalWarningCount} warnings) \n`; | |
} | |
const errorsText = `${totalErrorCount} ${totalErrorCount === 1 ? "error" : "errors"}`; | |
let warningsText = ""; | |
if (!quiet) { | |
warningsText = `, ${totalWarningCount} ${totalWarningCount === 1 ? "warning" : "warnings"}`; | |
} | |
summary += | |
`> ${totalErrorCount + totalWarningCount} problems ` + | |
`(${errorsText}${warningsText}) \n`; |
if (quiet) { | ||
this.#writeln( | ||
summaryColor( | ||
`${totalErrorCount} ${totalErrorCount === 1 ? "problem" : "problems"} ` + |
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.
Same as in markdown.ts
|
||
t.is(consoleLogStub.callCount, 0, "console.log should not be used"); | ||
t.true(processCwdStub.callCount > 0, "process.cwd was called"); | ||
t.is(processStdoutWriteStub.callCount, 2, "process.stdout.write was called twice"); | ||
t.is(processExitStub.callCount, 0, "process.exit got never called"); | ||
t.is(process.exitCode, 1, "process.exitCode was set to 1"); |
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.
What was the problem with the old exitCode
handling in this test?
Thank you for your contribution! 🙌
To get it merged faster, kindly review the checklist below:
Pull Request Checklist