Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat: suppress warnings flag for check and ci #4535

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

Conversation

samiam376
Copy link

@samiam376 samiam376 commented May 26, 2023

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.

#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

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented May 26, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 425a606
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6471372ee5931e0008776aba

@github-actions github-actions bot added the A-CLI Area: CLI label May 26, 2023
@ematipico
Copy link
Contributor

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 :)

@samiam376
Copy link
Author

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

@ematipico
Copy link
Contributor

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

fn process_messages(options: ProcessMessagesOptions) {

You want to create a mutable vector of Vec<Error>, where you will store all the diagnostics.

Every time you encounter a console.error

console.error(markup! {
{if verbose { PrintDiagnostic::verbose(&error) } else { PrintDiagnostic::simple(&error) }}
});

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants