-
Notifications
You must be signed in to change notification settings - Fork 23
Fixing bug where the worker thread would hang if there wasn't a tslint.json file #160
Conversation
lib/worker.js
Outdated
linter.lint(filePath, content, configuration); | ||
lintResult = linter.getResult(); | ||
} catch (err) { | ||
lintResult = {}; |
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.
Shouldn't something be done with this crash? (Logging to the console?)
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.
Yep, will add =]
spec/linter-tslint-spec.js
Outdated
|
||
it('validates even when there is no tslint.json', () => { | ||
waitsForPromise(() => | ||
atom.workspace.open(noConfigPath).then(editor => lint(editor)), |
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.
Should the results be checked?
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.
Not really sure. In this test I don't care that the file was accurately validated, nor the specific results, I just want to make sure that validation completed
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.
If that's all that was needed for the test, that's good enough for me.
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.
Inexplicably this line is blowing up and literally taking forever when configurationPath
is null
, which is the case in this new spec.
I'd recommend just adding && configurationPath
to the conditionals of the if
on the line above, as the entire block doesn't make a lot of sense if you have no configuration.
spec/linter-tslint-spec.js
Outdated
@@ -1,50 +1,48 @@ | |||
'use babel'; | |||
|
|||
import * as path from 'path'; | |||
import { beforeEach, it } from 'jasmine-fix'; // eslint-disable-line import/no-extraneous-dependencies |
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.
Put a note here about fit
, with async specs if you try to use fit
without importing it the spec will "pass" but do nothing.
Two issues in one:
Firstly, there was a regression I caused when copying the
rulesDirectory
code over to worker.js in #158. I missed the fact it used to refer tothis.
(which is nowconfig.
), so a circular reference was caused, which ledtslint
to have kittens, leading to the worker hanging.Secondly, if
tslint
hit an issue, it would cause the worker to hang.