Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

Update to support new linter APIs #16

Merged
merged 7 commits into from
Oct 15, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions lib/init.coffee
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'
Copy link
Member

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Install deps using atom-package-deps here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by the example - shouldn't require('atom-package-deps').install() take an optional callback instead of a string?
https://github.com/travs/atom-package-dependencies/blob/master/index.js#L6

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atom-package-dependencies vs atom-package-deps.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

path hasn't been imported, so it crashes on this line currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) =>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is tempfile really necessary? I mean does it not support stdin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK no,
cat test.js | gjslint gives

0 files checked, no errors found.

So, I suspect no :(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

google/closure-linter#96
We don't have to wait for it

params = [
'--nobeep',
'--quiet',
tmpFilePath
]
return helpers.exec(@executablePath, params, {cwd}).then (stdout) ->
regex = /^Line\s(\d*)\, E\:(\d*):\s(.*)$/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the \d* be \d+? It seems that if the message is in that format at all it would have a line number and error code.

(I'm not familiar with gjslint though, so who knows, maybe it does output Line , E:: stuff messages 😛)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you escape the first : for some reason, but not the second. : don't need to be escaped in JavaScript regex.

The , also doesn't need to be escaped.

Copy link
Member

Choose a reason for hiding this comment

The 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 (.+).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, although msg should be the entirety of the string, this doesn't work when you include the ^$, removing them causes it to successfully match against strings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do (E|W) instead of the E, you can specify Error/Warning instead of always saying everything is an error. (This would obviously require code changes below as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to follow linter's coffeelint.json, which would have you change this from == to is.


return [] unless lines.length
return lines.map (msg) ->
res = regex.exec(msg)
[all, line, code, text] = res
Copy link
Member

Choose a reason for hiding this comment

The 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],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might think about using helpers.rangeFromLineNumber to give a range that covers the entire content of the line, since gjslint doesn't give column numbers.

(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]
]
}
44 changes: 0 additions & 44 deletions lib/linter-gjslint.coffee

This file was deleted.

16 changes: 14 additions & 2 deletions package.json
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"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the package-deps section of the manifest? 😃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example here.

Choose a reason for hiding this comment

The 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"
}
}
}
}