Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore specified error in //@ts-ignore #21602

Closed
wants to merge 5 commits into from

Conversation

Busyrev
Copy link
Contributor

@Busyrev Busyrev commented Feb 3, 2018

Fixes #21197
Special syntax available now:

//@ts-ignore TS2349

to ignore only "Cannot invoke an expression whose type lacks a call signature." error.
Comma separated list supported, trailing comma is supported too:

//@ts-ignore TS2349, TS2552

If no error code specified, it ignores all errors.

@Busyrev
Copy link
Contributor Author

Busyrev commented Feb 3, 2018

sorry for linting issues, fixing them

@Jessidhia
Copy link

I actually suggested this in the original PR, but the author said he didn't like the idea because you can't tell what is being ignored by just the error code.

@@ -3,7 +3,7 @@
/// <reference path="core.ts" />

namespace ts {
const ignoreDiagnosticCommentRegEx = /(^\s*$)|(^\s*\/\/\/?\s*(@ts-ignore)?)/;
const ignoreDiagnosticCommentRegEx = /(^\s*$)|(^\s*\/\/\/?\s*(@ts-ignore)?(?:\s+((?:(?:TS\d+),\s*)*(?:TS\d+)?))?)/;

Choose a reason for hiding this comment

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

The ? in (?:TS\d+)? is redundant (causes an extra backtrack into a 0-length match, which is already allowed by the ? in the outer non-capturing match)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree, thanks, I'll remove it later

@Busyrev
Copy link
Contributor Author

Busyrev commented Feb 5, 2018

Error codes are public now, they printed berfore error message in error report. Imho blindly ignore everything is very dangerous and not good idea. Ignoring is always not good idea, but additional safety is not superfluous, we are at TypeScript repository, right?

@Busyrev
Copy link
Contributor Author

Busyrev commented Feb 5, 2018

Another suggestion:
I think it would be good idea to show error code in IDE, not only in text report. And it possible to show hint on rollover, like method signature or type information, when pointing on error code.

@lmcarreiro
Copy link

lmcarreiro commented Feb 15, 2018

Don't you have to split the provided string at the , (or /\s*,\s*/g) before use the indexOf?

We have cases like this:

TS9002 Only identifiers/qualified-names with optional type arguments are currently supported in a class 'extends' clause.

and

TS90020 Initialize property '{0}' in the constructor
TS90021 Initialize static property '{0}'
...
TS90029 ...

In this case, if we have a TS9002 and a comment // @ts-ignore TS90021, it would execute "TS90021".indexOf("TS9002") !== -1 and would return true instead of false

@Busyrev
Copy link
Contributor Author

Busyrev commented Feb 15, 2018

@lmcarreiro yes, you are right. I`ll fix it

@lmcarreiro
Copy link

Your regex doesn't match error codes with spaces before comma:
// @ts-ignore TS2349 , TS2552

@mhegazy
Copy link
Contributor

mhegazy commented Feb 27, 2018

Thanks @Busyrev for the contribution. We have had another heated discussion about this topic in the last language design meeting (see notes in #22011). The conclusion is we could not reach consensuses whether this feature should be added or not. It boils down to concerns about using error numbers, and how they do not add to expressiveness in a position like such. It is a discussion we have once every quarter or so, including when we first added support for ignoring errors. I am sure we will come to it again in the near future.
That all said, I do not think we would be able to take this PR in the time being. thanks again for the contribution and sorry for the delay.

@Busyrev
Copy link
Contributor Author

Busyrev commented Feb 27, 2018

@mhegazy no problem, I can use my own fork :). For discussion I think that short text error idents, like in tslint, would be better. But someone should create them.

@weswigham
Copy link
Member

weswigham commented Feb 28, 2018

For discussion I think that short text error idents, like in tslint, would be better. But someone should create them.

👍 Thank you 😍 This is one if the points we keep going back to 😉

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2018

closing for now.

@mhegazy mhegazy closed this Mar 2, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
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.

5 participants