-
Notifications
You must be signed in to change notification settings - Fork 23
Conversation
This seems to work great. I did a little timing experiment:
400ms seems to be the amount of time to start a new child process on my machine so there might be a noticeable difference on small files, but large coffeescript files are now usable and can be linted on change 👍 |
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.
Quick review just looking over the code, thanks for taking this on!
lib/Linter.coffee
Outdated
|
||
lint: (textEditor) -> | ||
filePath = textEditor.getPath() | ||
if filePath |
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.
You need an else
here returning []
(to clear messages) or null
(to tell Linter not to update any messages it might have for the file), otherwise you will return undefined
and trigger a Linter message to the user that an invalid result was returned.
lib/Linter.coffee
Outdated
startCol = (textEditor.getTabLength() * indentLevel) | ||
endCol = textEditor.getBuffer().lineLengthForRow(lineNumber - 1) | ||
|
||
range = [[lineNumber - 1, startCol], [lineNumber - 1, endCol]] |
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.
Let's just bring in atom-linter
and use generateRange
here like almost every other provider. This function has a much smarter version of this logic in place 😉.
lib/Linter.coffee
Outdated
position: range | ||
} | ||
|
||
return new Promise (resolve) -> |
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.
Same story with the if
above, you need to handle the Promise rejection and return a valid result ([]
or null
) to Linter.
lib/core.coffee
Outdated
|
||
canImportModules: (coffeelint) -> | ||
[major, minor, patch] = coffeelint.VERSION.split('.').map(toInt) | ||
if major > 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.
Any objection to just bringing in semver
and using it here?
lib/core.coffee
Outdated
|
||
coffeeLintPath = @_resolveCoffeeLint(filePath) | ||
# Versions before 1.9.1 don't work with atom because of an assumption 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.
'atom' -> 'Atom'
lib/core.coffee
Outdated
lineNumber: 1 | ||
level: 'error' | ||
message: 'CoffeeLint crashed, see console for error details.' | ||
rule: 'none' |
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.
I'm wondering if we should just let this throw a error instead of showing this on line 1
@UziTech It seems like your |
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.
There is some room for future improvement such as making the Task a permanently running worker that doesn't need to take the initialization hit on each launch, but this is a great start.
Thanks @UziTech!
Published in v1.3.0! 🎉 |
Run the core.coffee script as a Task in a separate process so it doesn't block the main UI thread on long files.
Fixes #87
TODO:
coffeelint
is used if available