Consume Linter-Plus

[steelbrain]

This is a meta bug about replacing AtomLinter with it's rewrite Linter-Plus, This rewrite features an extremely easy and flexible to use API along, which removes almost all of the limitations, including the complete project linting for hack.
Linter-Plus provides it's providers three types of messages

class LinterTrace
  constructor:(@Message, @File, @Position)
class LinterError
  constructor:(@Message, @File, @Position, @Trace)
class LinterWarning
  constructor:(@Message, @File, @Position, @Trace)

That, when consumed properly results like this

Fixes #1
Fixes #5
Fixes #6
Fixes #7
Fixes #8
Blocked on AtomLinter/linter-plus#6 (it's ready to ship, will be shipped in a few hours maybe?)

[bolinfest]

One objection I have to the linter-plus API is that it requires providers to use linter-plus's object model, which is awkward. Looking at https://github.com/steelbrain/Atom-Hack/blob/rewrite/lib/atom-hack.coffee, I'm not comfortable with how Trace or Error is injected. This makes it very hard to write an apm test for your provider.

By comparison, autocomplete-plus does not use inheritance or class-based types. It requires providers to return objects with properties of specific types. I think that is much cleaner.

[steelbrain]

I fail to think of a reason to choose {type:"Error", file:Message.File, position: Message.Position, message: Message.desc} over new Error(Message.desc, Message.File, Message.Position).
that Error class however at some points make the codebase of linter-plus simpler.

[bolinfest]

I should be able to write a unit test for my linter provider that does not depend on the consumer. If I were to do that today, I would have to mock out constructors for Error and Trace, which makes the test more tedious to write.

This is a real issue. Note that today we have to disable our tests for our FlowLinter because we have observed that tests that have to call activatePackage('linter') in beforeEach() are flaky:

https://github.com/facebook/nuclide/blob/9ed0ce4571baa7ba987118903f7ba3d46692191a/pkg/nuclide/flow/spec/FlowLinter-spec.js#L13-L14

This is because linter requires you to get a superclass from the Atom package itself:

https://github.com/facebook/nuclide/blob/9ed0ce4571baa7ba987118903f7ba3d46692191a/pkg/nuclide/flow/lib/FlowLinter.js#L13

The build flakiness is not worth it.

Also, passing in constructor functions is not the cleanest thing to do, IMHO. You could just as easily pass in some sort of factory with methods like createError(), etc., that would be cleaner. At least those would be easier to mock.

Further, your current contracts are not future-proof. That is, what happens when you want to start adding more parameters, more of which are optional? Your param list will become a mess. It would be easiest to represent each type as an object literal (at least from the provider's perspective) so that the consumer can recognize more properties over time, but older providers will still work.

In sum, let providers return object literals, and internally, you can convert them to Error, Trace, etc. Things will be more loosely coupled that way.

[steelbrain]

Thank you for providing a valid reason. I agree that Error and Trace constructors are not future proof, It would introduce complications when we add new parameters to constructors.

On a side note, createError isn't really the cleanest way to do this, so I am gonna make linter-plus accept Error like Objects very soon after the PR mentioned above has been accepted.

[steelbrain]

@bolinfest That new API has landed in the master branch. You can check out the new API in README. It's right according to what you suggested.

[steelbrain]

@bolinfest The original linter has been depreciated and will be removed soon, all packages are advised to switch to plus API.

[bolinfest]

Fixed in 1fc2358.

Though please take note of https://github.com/AtomLinter/linter-plus/issues/87.