[tslint] use exitCode 1 when linter errors #20567
Conversation
|
|
||
| /** | ||
| * Lints a list of files with eslint. eslint reports are written to the log | ||
| * Lints a list of files with tslint. tslint reports are written to the log |
| log.error('Unhandled execption!'); | ||
| log.error(error); | ||
| process.exit(1); | ||
| process.exit(); |
There was a problem hiding this comment.
Can you explain the reason for this way?
There was a problem hiding this comment.
Does setting process.exitCode have the side-effect that even if error.errors == true, it will exit with error?
There was a problem hiding this comment.
Yeah, setting process.exitCode tells node that when it naturally exits to use that exit code. I could also just call process.exit(1); in both branches of the .catch() handler, but I personally prefer to use the natural exit method as it's a little less brute-force and doesn't hide things like child processes or file descriptors that are left open and preventing the process from naturally exiting.
There was a problem hiding this comment.
That said, calling process.exit() in the case that an unhandled exception is caught is important, because an unhandled exception could mean that we hit a failure before we were able to properly clean up. But in the case below, were we are iterating error.errors, all cleanup should have run so this sorta relies on that and will fail to exit if that changes in the future (intentionally, hopefully preventing us from committing changes that break cleanup in some way)
There was a problem hiding this comment.
node's natural exit is nice to leverage here.
is there a reason we can't really follow the same pattern in this tslint script, as we do in eslint script? it looks like in that one, we throw error and let it bubble up. can we do something like that here? and handle the exit higher?
There was a problem hiding this comment.
We could if there was a higher-level component, but this is the highest level script for typescript
There was a problem hiding this comment.
See https://github.com/spalger/kibana/blob/e4e809180de56db64776b0b8767258f79afe19b8/scripts/tslint.js#L21. scripts/tslint calls this function directly and it is responsible for exposing the CLI interface
timroes
left a comment
timroes
left a comment
There was a problem hiding this comment.
Code looks good to me, and make sense (as long as we don't reset process.exitCode somewhere.
💚 Build Succeeded |
|
It seems to actually work: #20546 (comment) |
|
I won't have time to review this in the short term, but it looks like you have coverage. |
`node scripts/tslint` is used to run the linter on all projects, but isn't currently reporting a non-zero status code when there are failures. This PR ensures that the status code is always 1 when an error occurs in the linters. To test make a change to a TypeScript file that violates the linter rules and run `yarn grunt run:tslint` to make sure that `grunt` is seeing the non-zero exit code and fatal-ing correctly. ```sh yarn grunt run:tslint echo $? # should be 1 ```
|
6.4/6.x: d6a4331 |
5 participants
node scripts/tslintis used to run the linter on all projects, but isn't currently reporting a non-zero status code when there are failures. This PR ensures that the status code is always 1 when an error occurs in the linters.To test make a change to a TypeScript file that violates the linter rules and run
yarn grunt run:tslintto make sure thatgruntis seeing the non-zero exit code and fatal-ing correctly.