ignoreLintWarnings option #213
Conversation
Yes definitely 😄 Take a look at the current tests here: /~https://github.com/Realytics/fork-ts-checker-webpack-plugin/tree/master/test Hopefully there's a natural place for a new test to "fit" - unit or integration test I'm not sure. Hopefully an obvious candidate should present itself. |
|
@johnnyreilly added test. Let me know if it's not correct. |
|
Thanks! I'll take a look soon |
|
Hi @johnnyreilly, yeah that does look to have different functionality. |
|
Cool - I'd like to get yours in first. Could you update the |
src/index.ts
Outdated
| @@ -838,7 +841,7 @@ class ForkTsCheckerWebpackPlugin { | |||
| (this.lints || []).concat(this.diagnostics).forEach(message => { | |||
| const formattedMessage = this.formatter(message, this.useColors); | |||
|
|
|||
| message.isWarningSeverity() | |||
| message.isWarningSeverity() && !this.ignoreLintWarnings | |||
| ? this.logger.warn(formattedMessage) | |||
There was a problem hiding this comment.
I think this might be an issue in the case where you have a warning and you are ignoring warnings - it looks like an error would be logged in that case ..
There was a problem hiding this comment.
ah. So int that case I would want to do nothing here, right?
There was a problem hiding this comment.
Ok, updated. Let me know if that looks correct to you.
src/index.ts
Outdated
| @@ -808,6 +811,20 @@ class ForkTsCheckerWebpackPlugin { | |||
| return function noopEmitCallback() {}; | |||
| } | |||
|
|
|||
| private getLoggerMessage( | |||
There was a problem hiding this comment.
Thanks for this change - could I request one more tweak to avoid confusion please?
The name and the return type of the method suggests a logger message is passed back. In reality no value is ever returned I think? Could this be tweaked to make intention a bit clearer please?
There was a problem hiding this comment.
Oops yes, you're totally right. Will update.
|
|
|
Awesome - thanks for your work! Once the CI is happy I'll look to merge this and get a release out. 🤗 |
Attempts to address: #212
Adds a new option
ignoreLintWarnings.Question:
Should this have a test?
Thanks!