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

Add ban-ts-ignore rule. #3728

Merged
merged 6 commits into from
Aug 22, 2018
Merged

Add ban-ts-ignore rule. #3728

merged 6 commits into from
Aug 22, 2018

Conversation

bowenni
Copy link
Contributor

@bowenni bowenni commented Feb 21, 2018

PR checklist

Overview of change:

Add a new rule to ban "// @ts-ignore" comments.

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

CHANGELOG.md entry:

[new-rule] ban-ts-ignore

@bowenni
Copy link
Contributor Author

bowenni commented Feb 21, 2018

The failing test seems unrelated to my change?

@bowenni
Copy link
Contributor Author

bowenni commented Mar 2, 2018

Friendly ping :)

@johnwiseheart
Copy link
Contributor

Hi @bowenni - sorry for taking so long to review this PR. Theres a couple of new maintainers coming on to tslint - we're just ramping up to the codebase, then planning to fix up the failing test suite, and finally work through these PRs one by one and get them merged in. Thank you for your patience!

Until that time, a friendly reminder that you're able to use this rule locally as a custom rule in your tsconfig.json file.

no23reason
no23reason suggested changes Apr 21, 2018
View reviewed changes
Copy link
Contributor

@no23reason no23reason left a comment

Choose a reason for hiding this comment

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

One grammar comment, other than that LGTM

src/rules/banTsIgnoreRule.ts Outdated
};
/* tslint:disable:object-literal-sort-keys */

public static FAILURE_STRING = 'Do not use "// @ts-ignore" comments because it suppresses compilation errors.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar nit: Do not use "// @ts-ignore" comments because they suppress compilation errors.

@aervin
Copy link
Contributor

aervin commented Apr 21, 2018

@bowenni The ability to add code examples to rule documentation just landed in master, would you mind adding some for this rule? The best example of how to do this (on master) is the curly rule.

@bowenni
Copy link
Contributor Author

bowenni commented Apr 23, 2018 •
edited
Loading

Sure @aervin .
How do I generate the doc site locally?

@aervin
Copy link
Contributor

aervin commented Apr 23, 2018

Thanks @bowenni . We did some research on building docs locally in this thread. The picture of the result may be enough for you to go on without having to actually build the docs.

@bowenni
Copy link
Contributor Author

bowenni commented Apr 23, 2018

Thanks for the pointer @aervin .
I added a code example
oyypox20seh

aervin
aervin reviewed Apr 23, 2018
View reviewed changes
src/rules/banTsIgnoreRule.ts Outdated
@@ -0,0 +1,52 @@
/**
* @license
* Copyright 2013 Palantir Technologies, Inc.
Copy link
Contributor

@aervin aervin Apr 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Will you change the dates to 2018 in this file and the examples file? I think the other example files have this error too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

aervin
aervin reviewed Apr 23, 2018
View reviewed changes
src/rules/code-examples/banTsIgnore.examples.ts Outdated
"rules": { "ban-ts-ignore": true }
`,
pass: Lint.Utils.dedent`
console.log("hello");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think use the same code snippet from the fail example, but change the comment text to

// Compiler warns about unreachable code error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

giladgray
giladgray approved these changes May 1, 2018
View reviewed changes
Copy link

@giladgray giladgray left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(i pushed a commit to undo irrelevant copyright change)
looks good!

src/rules/code-examples/curly.examples.ts Outdated
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2013 Palantir Technologies, Inc.
Copy link

Choose a reason for hiding this comment

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

this change is not relevant. do not update copyright statements.

@bowenni
Copy link
Contributor Author

bowenni commented Jun 8, 2018

Friendly ping :)

@johnwiseheart
Copy link
Contributor

Hi @bowenni, can you please merge with master to ensure that the tests pass and then I can merge this.

@bowenni
Copy link
Contributor Author

bowenni commented Aug 22, 2018

Thanks @johnwiseheart. Done.

@johnwiseheart johnwiseheart merged commit 4702306 into palantir:master Aug 22, 2018
@johnwiseheart
Copy link
Contributor

Thanks for contributing!

@bowenni bowenni deleted the ban-ts-ignore branch August 22, 2018 22:02
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Nov 4, 2018
Closed
@mickaeltr
Copy link

Hello,
That's maybe a dumb question by I am wondering why I can't see this rule in the list?
My project is using TSLint 5.11.0
Thanks

@Antony74 Antony74 mentioned this pull request Dec 7, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aervin aervin aervin left review comments

@no23reason no23reason no23reason requested changes

@giladgray giladgray giladgray approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bowenni @johnwiseheart @aervin @mickaeltr @giladgray @no23reason