Fix: Remove duplicate error message on crash (fixes #8964) #10865
Conversation
nzakas
added
bug
| it("prints the error message pointing to line of code", () => { | ||
| const invalidConfig = `${__dirname}/../fixtures/bin/.eslintrc.yml`; | ||
| const child = runESLint(["--no-ignore", invalidConfig]); | ||
| const exitCodeAssertion = assertExitCode(child, 2); | ||
| const outputAssertion = getOutput(child).then(output => { | ||
| const expectedSubstring = "Error: bad indentation of a mapping entry at line"; | ||
| const expectedSubstring = ": bad indentation of a mapping entry at line"; |
There was a problem hiding this comment.
The format of this error message was different on my machine. It said "YAMLException:" instead of "Error:". I split the difference and left the colon in.
| it("prints the error message pointing to line of code", () => { | ||
| const invalidConfig = `${__dirname}/../fixtures/bin/.eslintrc.yml`; | ||
| const child = runESLint(["--no-ignore", invalidConfig]); | ||
| const exitCodeAssertion = assertExitCode(child, 2); | ||
| const outputAssertion = getOutput(child).then(output => { | ||
| const expectedSubstring = "Error: bad indentation of a mapping entry at line"; | ||
| const expectedSubstring = ": bad indentation of a mapping entry at line"; |
There was a problem hiding this comment.
Can you add an assertion here that the error message also contains the location of the invalid config file? e.g.
assert.include(output.stderr, path.resolve(invalidConfig));I vaguely remember that when I was looking at this, I was having trouble because the message "Cannot read config file: /path/to/config" wasn't appearing when I just printed error.stack. From testing this out locally it doesn't seem to be a problem anymore, but it would be good to have an assertion for it in case that changes in the future or it depends on the Node version.
There was a problem hiding this comment.
When I added that line, the test fails. There is no mention of the filename in the stack. I'd suggest opening a separate issue to track that. Agree?
There was a problem hiding this comment.
To clarify, was the filename previously getting printed for you before this change? Currently, the output for me (on master, with Node v10.10.0) looks like this:
$ eslint --no-ignore tests/fixtures/bin/.eslintrc.yml Makefile.js
Cannot read config file: /Users/tkatz/code/eslint/tests/fixtures/bin/.eslintrc.yml
Error: bad indentation of a mapping entry at line 5, column 9:
quotes: error
^
YAMLException: Cannot read config file: /Users/tkatz/code/eslint/tests/fixtures/bin/.eslintrc.yml
Error: bad indentation of a mapping entry at line 5, column 9:
quotes: error
^
at generateError (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
at throwError (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
at readBlockMapping (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1080:7)
at composeNode (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
at readDocument (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
at loadDocuments (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
at load (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
at Object.safeLoad (/Users/tkatz/code/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
at loadYAMLConfigFile (/Users/tkatz/code/eslint/lib/config/config-file.js:95:21)
at loadConfigFile (/Users/tkatz/code/eslint/lib/config/config-file.js:236:22)
To me, it seems like it's important for debugging that we tell users which config file has a problem when we report an "invalid config" error. So if the removal of console.error(err.message)also has the effect of removing this debugging info, then it seems like it might be worth leaving in the duplicate message for now (so we can make sure users continue to see the debugging info), until we have a better way to make sure that info is presented. On the other hand, if the name of the config file is already not appearing for you on master, then I'm fine with merging this PR now since it would be a clear improvement over the current situation.
In other words, my preference order for the possible outputs is:
- Print an error message once, with the location of the invalid config
- Print an error message twice, with the location of the invalid config
- Print an error message once, without the location of the invalid config
- Print an error message twice, without the location of the invalid config.
So I'm fine with merging this PR if it's moving us from state (4) to state (3), but I have concerns about it if it's moving us from state (2) to state (3).
There was a problem hiding this comment.
Gotcha, thanks for explaining. You are correct that error.message contains the filename while error.stack does not, so removing error.message will result in the filename not being output to the console.
(Side note: IIRC, this type of discrepancy is why I initially included both in the CLI. I actually don't think it's a problem to show duplicate error messages to get both the message and the stack, so I'm happy to just close the issue without merging this PR.)
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Removed the extra console output that included just the error message and updated the associated tests.
Is there anything you'd like reviewers to focus on?
No