Update to support new linter APIs #16

artoale · 2015-10-14T17:54:13Z

As reported in #10, #14 and #15, Linter APIs have changed.
This pull request is an attempt at fixing the issue.
It doesn't support ignored code - but it should at least work (since current implementation is not).
I suspect adding support for ignored codes would be good for another issue/pr.

Let me know if there's anything I should add/change.

Arcanemagus · 2015-10-14T18:05:14Z

Can you update the README.md as well? It still references things like needing to install linter manually 😉.

Arcanemagus · 2015-10-14T18:31:50Z

lib/init.coffee

+      lintOnFly: true
+      lint: (editor) =>
+        filePath = editor.getPath()
+        cwd = path.dirname(filePath)


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

I wonder why it's not on my local setup :) anyway - fixing now

Arcanemagus · 2015-10-14T19:05:28Z

I think that's all the comments I have for now, if you want here is a .diff of the changes required to get this working, but it doesn't address all of the comments above:

diff --git a/lib/init.coffee b/lib/init.coffee
index 82065ea..8ca5a44 100644
--- a/lib/init.coffee
+++ b/lib/init.coffee
@@ -22,6 +22,7 @@ module.exports =
     @subscriptions.dispose()
   provideLinter: ->
     helpers = require('atom-linter')
+    path = require 'path'
     provider =
       name: 'gjslint'
       grammarScopes: ['source.js', 'source.js.jquery', 'text.html.basic']
@@ -37,7 +38,7 @@ module.exports =
             tmpFilePath
           ]
           return helpers.exec(@executablePath, params, {cwd}).then (stdout) ->
-            regex = /^Line\s(\d*)\, E\:(\d*):\s(.*)$/
+            regex = /Line\s(\d+), E:(\d+):\s(.+)/
             lines = stdout.split('\n').filter (line) ->
               line.indexOf('Line') == 0

Arcanemagus · 2015-10-14T21:04:03Z

Btw, when this gets eventually gets merged make sure to file an issue to get this re-instated over here: https://github.com/AtomLinter/atomlinter.github.io

* Remove unnecessary escaping in RegEx * Always match at least one digit/character * Require path before using it :) * Use `is` instead of `==` * Include error code in the output message * Return whole line instead of first char as range

artoale · 2015-10-15T09:10:05Z

@Arcanemagus I've updated the PR to (hopefully) cover all your comments

Please let me know if there's anything else I should be doing apart from reporting to atomlinter.github.io once this gets eventually merged.

A bit OT: is there a "standard" way to disable the plugin live? (As you might expect, not all the project I work on require gjslint validation, and what I usually have to do is disable the relevant linter and restart Atom. If there's a standard, more user friendly way, that requires code changed let me know, I'd be happy to create a new PR - otherwise would a plugin-specific setting work in your opinion? It should remove the unnecessary step of restarting Atom.

Arcanemagus · 2015-10-15T16:18:26Z

README.md

-$ apm install linter-gjslint
-```
+## Installation
+Run `amp install linter-gjslint` or search for `linter-gjslint` in atom package manager.


Arcanemagus · 2015-10-15T16:24:37Z

Some plugins have a setting that, when enabled, disable linting if a configuration file isn't found. That really only helps if the linter itself uses configuration files though.

You shouldn't need to restart Atom btw, just disabling the plugin should work (now that it's moved to the new linter API, the old API would always lint since it was somewhat running itself).

Arcanemagus · 2015-10-15T16:26:28Z

@steelbrain this looks good to me (other than the slight README.md typo), any objections to merging this that you have? Also I would suggest that a team be created for this repo and @artoale be added as a maintainer, assuming you would want that @artoale 😉.

artoale · 2015-10-15T16:31:20Z

@Arcanemagus Thanks for the info - I haven't needed the "disable" feature since the new linter API, well, because of the purpose of this PR :)

I'll be very happy to help in maintaining this linter, if I can. I suspect it shouldn't require a massive effort in terms of time, so very happy to collaborate if needed!

steelbrain · 2015-10-15T16:35:43Z

lib/init.coffee

+      lint: (editor) =>
+        filePath = editor.getPath()
+        cwd = path.dirname(filePath)
+        tempFile path.basename(filePath), editor.getText(), (tmpFilePath) =>


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

AFAIK no,
cat test.js | gjslint gives

0 files checked, no errors found.

So, I suspect no :(

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

steelbrain · 2015-10-15T16:37:14Z

Also you can completely drop the donation part in the readme

steelbrain · 2015-10-15T16:38:25Z

@artoale You should've recieved the github emails

artoale · 2015-10-15T17:02:53Z

@steelbrain Yeah, got the invitation :)

steelbrain · 2015-10-15T17:04:11Z

LGTM. Merge at will.

Support new linter APIs

artoale · 2015-10-15T17:07:49Z

@steelbrain should I publish as well? if so, are we following a specific "definition" of how to use semver? (e.g. here we don't really change the API exposed - since well, we're just consuming some)

steelbrain · 2015-10-15T17:08:39Z

To publish a release clone the repo and do apm publish {major, minor, patch}. Major one would be cool as it's a complete rewrite.

Arcanemagus · 2015-10-15T17:15:51Z

Btw after doing an apm publish you can go here to GitHub and edit the tag that creates to mark it as the latest release here as well (since apm doesn't do that for some reason).

artoale · 2015-10-16T08:02:04Z

Oh, I see! Makes sense. Will do!

On Thu, Oct 15, 2015, 18:15 Landon Abney notifications@github.com wrote:

Btw after doing an apm publish you can go here to GitHub and edit the tag
that creates to mark it as the latest release here as well (since apm
doesn't do that for some reason).

—
Reply to this email directly or view it on GitHub
#16 (comment)
.

artoale added 2 commits October 14, 2015 18:34

Update linter to comply to new APIs

effa93e

Remove console.log references

714b41a

Arcanemagus reviewed Oct 14, 2015
View reviewed changes

artoale added 2 commits October 15, 2015 09:55

Update implementation based on PR comments

ff0b3ac

* Remove unnecessary escaping in RegEx * Always match at least one digit/character * Require path before using it :) * Use `is` instead of `==` * Include error code in the output message * Return whole line instead of first char as range

Update README.md

de59525

Arcanemagus reviewed Oct 15, 2015
View reviewed changes

Fix typo in README.md

02096a9

steelbrain reviewed Oct 15, 2015
View reviewed changes

steelbrain mentioned this pull request Oct 15, 2015

STDIN Support google/closure-linter#96

Open

artoale added 2 commits October 15, 2015 17:58

Remove references to donations in README.md

a79c733

Install deps with atom-package-deps

63f3579

artoale added a commit that referenced this pull request Oct 15, 2015

Merge pull request #16 from artoale/master

6e57034

Support new linter APIs

artoale merged commit 6e57034 into AtomLinter:master Oct 15, 2015

artoale mentioned this pull request Oct 15, 2015

Upcoming linter changes #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to support new linter APIs #16

Update to support new linter APIs #16

artoale commented Oct 14, 2015

Arcanemagus commented Oct 14, 2015

Arcanemagus Oct 14, 2015

artoale Oct 15, 2015

Arcanemagus commented Oct 14, 2015

Arcanemagus commented Oct 14, 2015

artoale commented Oct 15, 2015

Arcanemagus Oct 15, 2015

Arcanemagus commented Oct 15, 2015

Arcanemagus commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain Oct 15, 2015

artoale Oct 15, 2015

steelbrain Oct 15, 2015

steelbrain commented Oct 15, 2015

steelbrain commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain commented Oct 15, 2015

Arcanemagus commented Oct 15, 2015

artoale commented Oct 16, 2015

Update to support new linter APIs #16

Update to support new linter APIs #16

Conversation

artoale commented Oct 14, 2015

Arcanemagus commented Oct 14, 2015

Arcanemagus Oct 14, 2015

Choose a reason for hiding this comment

artoale Oct 15, 2015

Choose a reason for hiding this comment

Arcanemagus commented Oct 14, 2015

Arcanemagus commented Oct 14, 2015

artoale commented Oct 15, 2015

Arcanemagus Oct 15, 2015

Choose a reason for hiding this comment

Arcanemagus commented Oct 15, 2015

Arcanemagus commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain Oct 15, 2015

Choose a reason for hiding this comment

artoale Oct 15, 2015

Choose a reason for hiding this comment

steelbrain Oct 15, 2015

Choose a reason for hiding this comment

steelbrain commented Oct 15, 2015

steelbrain commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain commented Oct 15, 2015

artoale commented Oct 15, 2015

steelbrain commented Oct 15, 2015

Arcanemagus commented Oct 15, 2015

artoale commented Oct 16, 2015