Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

enhancement: Add quiet flag to hide warnings #4025

Merged
merged 1 commit into from
Jul 18, 2018
Merged

enhancement: Add quiet flag to hide warnings #4025

merged 1 commit into from
Jul 18, 2018

Conversation

MatthewHerbst
Copy link
Contributor

@MatthewHerbst MatthewHerbst commented Jul 10, 2018

Sometimes you may want to run linting but ignore any warnings produced, such as in a CI environment where you only want lint failures to fail the build (many runners will check both the exit code and STDERR to see if the program has passed).

This mimics the quiet flag available in ESLint, which hides errors from output when passed.

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Adds a new CLI flag, quiet, to allow hiding of lint warnings.

Is there anything you'd like reviewers to focus on?

What's the best way to test this? I've been having trouble trying to do it in the linterTests. I've been trying:

const withWarningDeclaration =
`
  console.log("This line will not pass linting with the default rule set");
`;

it("shows warnings", () => {
    const config = DEFAULT_CONFIG;
    config.rules.set("no-console", {
        ruleArguments: ["log"],
        ruleName: "no-console",
        ruleSeverity: "warning",
    });
    
    const linter = new TestLinter({ fix: false });
    const fileToLint = createTempFile("ts");
    fs.writeFileSync(fileToLint, withWarningDeclaration);
    linter.lint(fileToLint, "Don't Need This If We Have A Program", config);
    const result = linter.getResult();

    assert.equal(result.warningCount, 1);
    assert.equal(result.errorCount, 0);
});

describe("when the `quiet` flag is passed", () => {
    ...
    it("does not show warnings", () => {
        ...
    });
});

But that doesn't seem to produce any warnings or errors. Any idea why?

CHANGELOG.md entry:

enhancement: added quiet flag to allow hiding of lint warnings

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @MatthewHerbst! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@MatthewHerbst
Copy link
Contributor Author

@johnwiseheart (pinging you since I've seen you active on a few other PRs recently): any chance I could please get any feedback on this? Thank you!

Copy link
Contributor

@johnwiseheart johnwiseheart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to look into your testing issue, and will get back to you.

src/linter.ts Outdated
@@ -142,6 +142,10 @@ export class Linter {
}

public getResult(): LintResult {
if (this.options.quiet) {
this.failures = this.failures.filter((failure) => failure.getRuleSeverity() === "error");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use a new variable here rather than overwriting this.failures - perhaps something like:

const errors = this.failures.filter((failure) => failure.getRuleSeverity() === "error");
const failures = this.options.quiet ? errors : this.failures;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done!

@johnwiseheart
Copy link
Contributor

@MatthewHerbst with regard to your test:

it("shows warnings", () => {
    const config = DEFAULT_CONFIG;
    config.rules.set("no-console", {
        ruleArguments: ["log"],
        ruleName: "no-console",
        ruleSeverity: "warning",
    });

    const linter = new TestLinter({ fix: false });  // <---- You're not passing in a program
    const fileToLint = createTempFile("ts");
    fs.writeFileSync(fileToLint, withWarningDeclaration);
    linter.lint(fileToLint, withWarningDeclaration, config); // <-- So when you call lint you need to pass the file contents
    const result = linter.getResult();

    assert.equal(result.warningCount, 1);
    assert.equal(result.errorCount, 0);
});

After passing in the contents of the file, your code works for me. That said, I'm not 100% confident that I'm right since my TSLint knowledge is fairly basic. Please correct me if I'm wrong.

@johnwiseheart
Copy link
Contributor

Also, please rebase on top of master to fix the tests

@MatthewHerbst
Copy link
Contributor Author

Ah, I see, I messed up thinking about the Program was, thank you! Will fix it now

Sometimes you may want to run linting but ignore any warnings
produced, such as in a CI environment where you only want lint
failures to fail the build (many runners will check both the exit
code and STDERR to see if the program has passed).

This mimics the `quiet` flag available in ESLint, which hides
errors from output when passed.
@MatthewHerbst
Copy link
Contributor Author

@johnwiseheart any other changes you'd like to see on this? Thanks again for the reviews!

@johnwiseheart
Copy link
Contributor

Looks good to me! Thanks for your contribution!

@johnwiseheart johnwiseheart merged commit 6b52df3 into palantir:master Jul 18, 2018
@MatthewHerbst MatthewHerbst deleted the feature/quiet-flag branch July 18, 2018 21:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants