-
Notifications
You must be signed in to change notification settings - Fork 12
Update to support new linter APIs #16
Changes from 2 commits
effa93e
714b41a
ff0b3ac
de59525
02096a9
a79c733
63f3579
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,57 @@ | ||
{CompositeDisposable} = require 'atom' | ||
{findFile, exec, tempFile} = helpers = require 'atom-linter' | ||
|
||
module.exports = | ||
config: | ||
gjslintExecutablePath: | ||
executablePath: | ||
type: 'string' | ||
default: '' | ||
title: 'gjslint executable path' | ||
default: '/usr/local/bin/gjslint' | ||
gjslintIgnoreList: | ||
type: 'array' | ||
default: [] | ||
items: | ||
type: 'string' | ||
|
||
activate: -> | ||
console.log 'activate linter-gjslint' | ||
@subscriptions = new CompositeDisposable | ||
@subscriptions.add atom.config.observe 'linter-gjslint.executablePath', | ||
(executablePath) => | ||
@executablePath = executablePath | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Install deps using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused by the example - shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
deactivate: -> | ||
@subscriptions.dispose() | ||
provideLinter: -> | ||
helpers = require('atom-linter') | ||
provider = | ||
name: 'gjslint' | ||
grammarScopes: ['source.js', 'source.js.jquery', 'text.html.basic'] | ||
scope: 'file' | ||
lintOnFly: true | ||
lint: (editor) => | ||
filePath = editor.getPath() | ||
cwd = path.dirname(filePath) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why it's not on my local setup :) anyway - fixing now |
||
tempFile path.basename(filePath), editor.getText(), (tmpFilePath) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is tempfile really necessary? I mean does it not support stdin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK no,
So, I suspect no :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. google/closure-linter#96 |
||
params = [ | ||
'--nobeep', | ||
'--quiet', | ||
tmpFilePath | ||
] | ||
return helpers.exec(@executablePath, params, {cwd}).then (stdout) -> | ||
regex = /^Line\s(\d*)\, E\:(\d*):\s(.*)$/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the (I'm not familiar with gjslint though, so who knows, maybe it does output There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also you escape the first The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to the first comment, the message will probably contain at least one character, so that should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, although There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the (E|W) I suspect there's no rule reported as warning by gjslint, but I'll double check on their codebase There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, as you can see from erroroutput.py:46 reported errors have an "Error" status, always. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, just wanted to make sure as the old version checked for both for some reason. |
||
lines = stdout.split('\n').filter (line) -> | ||
line.indexOf('Line') == 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to follow |
||
|
||
return [] unless lines.length | ||
return lines.map (msg) -> | ||
res = regex.exec(msg) | ||
[all, line, code, text] = res | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to include the code in your message, to make looking up the error simpler if the user is unfamiliar with it or it is not showing results they are expecting. |
||
line = parseInt(line, 10) | ||
return { | ||
text: text, | ||
type: 'error', | ||
filePath: filePath, | ||
range: [ | ||
[line - 1, 0], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might think about using (Just using the first column is fine, it just means you get no inline markers and when a user clicks on your message the inline tooltip, if enabled, shows at the start of the line instead of where the code is.) |
||
[line - 1, 1] | ||
] | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,25 @@ | ||
{ | ||
"name": "linter-gjslint", | ||
"main": "./lib/init", | ||
"linter-package": true, | ||
"version": "0.0.5", | ||
"description": "Linter plugin for JavaScript, using gjslint (Google Closure Linter)", | ||
"repository": "https://github.com/AtomLinter/linter-gjslint", | ||
"license": "MIT", | ||
"engines": { | ||
"atom": ">0.50.0" | ||
}, | ||
"dependencies": {} | ||
"dependencies": { | ||
"atom-linter": "^3.2.0", | ||
"atom-package-deps": "^2.0.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Example here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disregard, just saw it a few lines below this one. |
||
}, | ||
"package-deps": [ | ||
"linter" | ||
], | ||
"providedServices": { | ||
"linter": { | ||
"versions": { | ||
"1.1.0": "provideLinter" | ||
} | ||
} | ||
} | ||
} |
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.
It might be a better idea to just have the default be
gjslint
, that way if it's in the user's path anywhere it should be found automatically, with them having the option to specify the full path if necessary.