-
-
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: Ensure piped in code will trigger correct errors (fixes #2154) #2162
Conversation
} catch (ex) { | ||
console.error(ex.message); | ||
exitCode = 1; | ||
} |
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.
Wouldn’t this error also end up in the error
event of the stream? Anyway it is always a good idea to add an error handler to every error event (https://github.com/maxogden/concat-stream#error-handling).
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.
No, the error is thrown in the callback, not in the process of reading the stream.
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.
Interesting, but somewhere the error gets caught.
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.
Yup, I just have no idea where and not enough patience to keep digging in. :)
Maybe we should try to extract this functionality to a different file where we can write tests more easily or we should write tests for |
If you want to take a stab at writing tests, I'm all for it. I just don't have the time to figure out the best way to do it (this is part of why we have |
Fix: Ensure piped in code will trigger correct errors (fixes #2154)
Alright, I will investigate how we could test this. |
This is a bit gross.
bin/eslint.js
is the one file we don't have unit tests for, but I verified this is working.The error was being swallowed because the method was being called in a callback.