Skip to content

feat: add eslintQuiet option (#340) #346

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

Closed
wants to merge 4 commits into from

Conversation

jeffshaver
Copy link

This PR adds a quiet option so that warnings get suppressed in the output.

I definitely would like feedback on if this was the right approach. But also, when I went to write an integration test for this, I couldn't get it working correctly. when incrementalTypsciptApi is on, i get warnings like:

{
   rawMessage: '\u001b[1m\u001b[31mERROR in \u001b[39m\u001b[22m\u001b[
1m\u001b[36mundefined(undefined,undefined)\u001b[39m\u001b[22m\u001b[1m\u001b[
31m:\u001b[39m\u001b[22m\n' +
    "\u001b[90mTS5055: \u001b[39mCannot write file '/test-context/src/lib/otherFunc.js' because it would overwrite input file.",
  message: '\u001b[1m\u001b[31mERROR in \u001b[39m\u001b[22m\u001b[1m\u001b[36mundefined(undefined,undefined)\u001b[39m\u001b[22m\u001b[1m\u001b[31m:\u001b[39m\u001b[22m\n' +
    "\u001b[90mTS5055: \u001b[39mCannot write file '/test-context/src/lib/otherFunc.js' because it would overwrite input file.",
  location: { line: undefined, character: undefined },
  file: undefined
}

@@ -37,9 +37,13 @@ export function createEslinter(eslintOptions: object) {
| IterableIterator<eslinttypes.CLIEngine.LintReport>
| eslinttypes.CLIEngine.LintReport[]
) {
const eslint: typeof eslinttypes = require('eslint');
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to re-require ESLint here? We require it earlier in createEslinter. Is that no longer used with the new approach?

Copy link
Author

Choose a reason for hiding this comment

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

No, this was an accidental add i think. My bigger questions are with how to test any of this since just adding a test caused tests to start failing.

@johnnyreilly
Copy link
Member

johnnyreilly commented Nov 10, 2019

The approach seems fine; you're using https://eslint.org/docs/developer-guide/nodejs-api#clienginegeterrorresults which looks to be a good call. Why the integration test is failing is mysterious... It seems to be an issue with writing to disk:

Cannot write file '/test-context/src/lib/otherFunc.js' because it would overwrite input file.

@johnnyreilly
Copy link
Member

I thought my change (creating a new fixture) would fix the tests - it seems not 😢

The issue seems to be related to files in the test context not being cleaned up. Most perplexing.

@piotr-oles
Copy link
Collaborator

I'm closing it as I was able to release v5.0.0-alpha.1 version which allows you to specify issues predicates :)

@piotr-oles piotr-oles closed this May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants