tools
tools copied to clipboard
feat: suppress warnings flag for check and ci
Summary
When linting a large number a files its useful to prioritize the errors in the output. Allowing the user to suppress the warning messages makes it easier to address the higher priority errors first. Similarly on CI the user may not care about seeing the warning messages since they will not lead to a CI failure and thus just add noise to the logs.
This is my first pr on the repo so let me know if you think there is a more trivial way to implement this.
https://github.com/rome/tools/discussions/4532
Test Plan
Added test cases to the check and ci commands in /crates/rome_cli/tests/commands
cargo test --package rome_cli --test main -- commands::check::suppress_warnings --exact --nocapture
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 146 filtered out; finished in 0.20s```
cargo test --package rome_cli --test main -- commands::ci::suppress_warnings --exact --nocapture
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 146 filtered out; finished in 0.09s```
Changelog
- [x] The PR requires a changelog line
Documentation
- [x] The PR requires documentation
- [x] I will create a new PR to update the documentation
Deploy Preview for docs-rometools canceled.
Built without sensitive environment variables
| Name | Link |
|---|---|
| Latest commit | 425a6063c210740c5b84ca966d8203c65892c320 |
| Latest deploy log | https://app.netlify.com/sites/docs-rometools/deploys/6471372ee5931e0008776aba |
Hey @samiam376 , sorry if I didn't follow up on the discussion. Could you let me know if you're sure this is the feature we wanted to implement? The initial suggestion was to order the diagnostics based on severity.
It seems now that the suggested feature is different :)
Hey @samiam376 , sorry if I didn't follow up on the discussion. Could you let me know if you're sure this is the feature we wanted to implement? The initial suggestion was to order the diagnostics based on severity.
It seems now that the suggested feature is different :)
@ematipico - after looking through the codebase this seemed like it would be an easier way to achieve a similar goal. I wanted to ability to see errors first on checks/ci and simply suppressing nonerrors is a simpler/more performant solution.
I would be open to working on the ordering as well, but I think it would warrant more design considerations for the library
Hey @samiam376 , sorry if I didn't follow up on the discussion. Could you let me know if you're sure this is the feature we wanted to implement? The initial suggestion was to order the diagnostics based on severity. It seems now that the suggested feature is different :)
@ematipico - after looking through the codebase this seemed like it would be an easier way to achieve a similar goal. I wanted to ability to see errors first on checks/ci and simply suppressing nonerrors is a simpler/more performant solution.
I would be open to working on the ordering as well, but I think it would warrant more design considerations for the library
I believe this can be implemented without changing too much of the existing code and the change will be focused on a particular part of the CLI, and considering that we plan to implement a CLI argument called --fail-on-warnings, this could go in conflict with --suppress-warnings.
This is the function that is responsible to write diagnostics to console
https://github.com/rome/tools/blob/8fc4c6f9a7c3d3d2eb8a27cce727c520876ac6fe/crates/rome_cli/src/execute/traverse.rs#L273
You want to create a mutable vector of Vec<Error>, where you will store all the diagnostics.
Every time you encounter a console.error
https://github.com/rome/tools/blob/8fc4c6f9a7c3d3d2eb8a27cce727c520876ac6fe/crates/rome_cli/src/execute/traverse.rs#L337-L339
You'd want to store that diagnostic in the vector
diagnositcs_to_print.push(error);
Sometimes diagnostics are not of type Error, but you can use .into() or Error::from to achieve that
diagnositcs_to_print.push(diff_diagnostic.into());
diagnositcs_to_print.push(Error::from(diff_diagnostic));
The printing will happen after the while loop. There, we will do something like this:
diagnositcs_to_print.sort_by(|a, b| {
a.severity().partial_cmp(&b.severity())
});
for error in diagnositcs_to_print {
if mode.should_report_to_terminal() && should_print {
console.error(markup! {
{if verbose { PrintDiagnostic::verbose(&error) } else { PrintDiagnostic::simple(&error) }}
});
}
}
As a bonus - which means we don't need to it here, but if you're up to the task, feel free to go for it - we could pass a CLI argument, something like --order-diagnostics=<asc|desc>, which will sort the diagnostics based on its value.