Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible for a linter to register itself via atom.services. #432

Closed
wants to merge 598 commits into from

Conversation

bolinfest
Copy link

The goal is for clients of linter to avoid the following declaration
that they have to do today:

var Linter = require(atom.packages.getLoadedPackage('linter').path + '/lib/linter');

I don't know whether this change is a good idea, but I tested it and
it does [mostly] work. This amends package.json to declare the following
services:

  "consumedServices": {
    "linter.addLinterClass": {
      "versions": {
        "0.1.0": "addLinterClass"
      }
    }
  },
  "providedServices": {
    "linter.provideLinterClass": {
      "versions": {
        "0.1.0": "provideLinterClass"
      }
    }
  },

A client would then add the following to its package.json:

  "consumedServices": {
    "linter.provideLinterClass": {
      "versions": {
        "0.1.0": "createSubclassForLinterClass"
      }
    }
  },

which it would implement as:

'use 6to5';

module.exports = {
  createSubclassForLinterClass(linterClass) {
    class MyLinter extends linterClass {

    }
    MyLinter.prototype.cmd = 'some command';
    MyLinter.prototype.linterName = 'my-linter';
    MyLinter.prototype.regex = /some regex/;
    MyLinter.syntax = 'source.js';

    atom.services.provide(
        'linter.addLinterClass',
        '0.1.0',
        MyLinter);
  },
};

The major drawback to this scheme (other than it being weird) is
that MyLinter will be registered after the initial set of
LinterViews have been created. Therefore, messages from MyLinter
will not be shown in existing editors, but they will be shown in
any new editors that are created.

This could be worked out if this scheme were determined to be worth pursuing.

An alternative approach to avoiding the atom.packages.getLoadedPackage('linter').path
call would be to offer Linter via npm so it can be require()'d directly.

This would also make it easier for clients of linter to unit test their code.

Steve Teuber and others added 30 commits August 21, 2014 15:14
Fixes steelbrain#190. Will eliminate another class of `span ENOTDIR` errors.

Before this change, `executablePath` was assumed to be a directory. A common
configuration error was set it to put the full path to the linter executable.
There's no harm in making this use case work, though.

Test Plan:
Ran specs
Separates out two independent pieces of logic

Test Plan:
Ran specs
Some linters are sensitive to filenames. See [steelbrain#191][191] and `linter-rubocop`'s
'[steelbrain#2][2]. They might be happier if we use a temporary directory instead of a
temporary file.

  [2]: AtomLinter/linter-rubocop#2
  [191]: steelbrain#191

Test Plan:

Ran lint on a CoffeeScript file and it still works. Didn't test if Haskell &
Ruby linters are happier.
Use temporary directory instead of temporary file
steelbrain#193
It was returning after one linter was applied.
Fix for multiple linter regression
This fix is better because it achieves what I was originally trying to do with
that `return` statement - stop CoffeeScript from accumalting the implicit
returns for the `for` loop into an array.

Test Plan:
Ran new test before and after fix. It works!
Show lint messages inline, next to the code that it applies to.  See
the new gif in the readme for an example.
I added this because I otherwise had no idea why a particular linter was not returning on a large file
…imeout

Added debug log for process timeout
Test Plan:
Changed multiplier from 1000 to 10 and reloaded window. Saw linters time out.
Fixes steelbrain#202.

Also is better because it doesn't make the gutter wider. Note that this
occupies the same spot as the fold marker, but that only shows up on hover
anyway, and it still appears.

Another change here is to begin using @text-color-warning and @text-color-error
which are provided by the theme.

Test Plan:
Looked at both error and warning level lint messages.
Test Plan:
Reloaded window; lint messages still appear
Test Plan:
Loaded window with both kinds of lint messages and it looked reasonable
dmnd and others added 21 commits January 31, 2015 23:08
Recognize info match in regexp and set message level
This follows the first suggestion on
https://discuss.atom.io/t/how-to-speed-up-your-packages/10903.

Previous to this diff, `linter` would take over 200ms to load on my
machine, according to Timecop. Now when I run:

    atom.packages.getLoadedPackage('linter').loadTime

I consistently see `linter` loading in under 10ms (it's usually <5ms).

Activation time has not seemed to increase dramatically as a result:

    atom.packages.getLoadedPackage('linter').activateTime

Seems to hover around 80ms.

I can probably do even better in follow-up diffs.
Push top-level require() calls down into the functions that use them.
This makes it possible to remove a dependency on `lodash` in `init.coffee`.

Curiously, this was my first justified use of the
[Embedded JavaScript](http://coffeescript.org/#embedded) feature in
CoffeeScript because I needed to use the ES6 iteration idiom for `Set`,
for which there is no equivalent in CoffeeScript.
Both `CompositeDisposable` and `Emitter` are available on `require('atom')`.

I even made sure that `require('event-kit').Emitter === require('atom').Emitter`
was `true`.
Change linterViews to be an ES6 Set rather than an Array.
The goal is for clients of `linter` to avoid the following declaration
that they have to do today:

    var Linter = require(atom.packages.getLoadedPackage('linter').path + '/lib/linter');

I don't know whether this change is a good idea, but I tested it and
it does [mostly] work. This amends `package.json` to declare the following
services:

```
  "consumedServices": {
    "linter.addLinterClass": {
      "versions": {
        "0.1.0": "addLinterClass"
      }
    }
  },
  "providedServices": {
    "linter.provideLinterClass": {
      "versions": {
        "0.1.0": "provideLinterClass"
      }
    }
  },
```

A client would then add the following to its `package.json`:

```
  "consumedServices": {
    "linter.provideLinterClass": {
      "versions": {
        "0.1.0": "createSubclassForLinterClass"
      }
    }
  },
```

which it would implement as:

```
'use 6to5';

module.exports = {
  createSubclassForLinterClass(linterClass) {
    class MyLinter extends linterClass {

    }
    MyLinter.prototype.cmd = 'some command';
    MyLinter.prototype.linterName = 'my-linter';
    MyLinter.prototype.regex = /some regex/;
    MyLinter.syntax = 'source.js';

    atom.services.provide(
        'linter.addLinterClass',
        '0.1.0',
        MyLinter);
  },
};
```

The major drawback to this scheme (other than it being weird) is
that `MyLinter` will be registered after the initial set of
`LinterView`s have been created. Therefore, messages from `MyLinter`
will not be shown in existing editors, but they will be shown in
any new editors that are created.

This could be worked out if this scheme were determined to be worth pursuing.

An alternative approach to avoiding the `atom.packages.getLoadedPackage('linter').path`
call would be to offer `Linter` via npm so it can be `require()`'d directly.

This would also make it easier for clients of `linter` to unit test their code.
@bolinfest
Copy link
Author

Has there been any discussion around moving linter.coffee to npm? I concede that maintaining both an apm package and an npm package is more work, but it's probably simpler for developers (modulo version skew).

One other approach would be to favor composition over inheritance. That is, if the client provided some object that was composed by Linter rather than one that extended it, it would decouple things further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet