Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

marianfoo
Copy link
Contributor

Thank you for your contribution! 🙌

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

@marianfoo
Copy link
Contributor Author

had some trouble with exitCode in the test, don´t know if this was correctly handled

@flovogt flovogt self-assigned this Apr 16, 2025
@flovogt
Copy link
Member

flovogt commented Apr 16, 2025

Thanks a lot for your contribution @marianfoo. We will take care of this PR in backlog item CPOUI5FOUNDATION-837.

@flovogt flovogt added the configuration An issue related to configuration capabilities label Apr 16, 2025
fixableWarningCount: 0,
}));
};

Copy link
Member

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,
Copy link
Member

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?

Comment on lines +68 to +76
if (quiet) {
summary +=
`> ${totalErrorCount} ${totalErrorCount === 1 ? "problem" : "problems"} ` +
`(${totalErrorCount} ${totalErrorCount === 1 ? "error" : "errors"}) \n`;
} else {
summary +=
`> ${totalErrorCount + totalWarningCount} problems ` +
`(${totalErrorCount} errors, ${totalWarningCount} warnings) \n`;
}
Copy link
Member

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?

Suggested change
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"} ` +
Copy link
Member

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");
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration An issue related to configuration capabilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants