tools icon indicating copy to clipboard operation
tools copied to clipboard

feat: suppress warnings flag for check and ci

Open samiam376 opened this issue 2 years ago • 4 comments

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

samiam376 avatar May 26 '23 22:05 samiam376

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

netlify[bot] avatar May 26 '23 22:05 netlify[bot]

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 avatar May 30 '23 13:05 ematipico

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

samiam376 avatar May 30 '23 17:05 samiam376

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.

ematipico avatar Jun 13 '23 05:06 ematipico