-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Fix: Remove duplicate error message on crash (fixes #8964) #10865
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I just have a minor suggestion for the test.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
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