search

gustavnikolaj / linter-js-standard-engine

Linter plugin for Standard Engine based linters.
ISC License
10 stars 6 forks source link

Don't select linter just because there is config for it #19

Closed novemberborn closed 8 years ago

novemberborn commented 8 years ago

Follow-up from https://github.com/gustavnikolaj/linter-js-standard-engine/pull/15#issuecomment-256896370:

Also, right now a linter is selected if config is present, even if it's not in the devDependencies. I think it'd be good to require the linter to either be in devDependencies or in standard-engine.

gustavnikolaj commented 8 years ago

This might be clashing somewhat with #18. If we don't resolve the linter before we start the worker, we don't know if we're actually going to lint anything.

Maybe the key is to have the atom-part do the checking of package.json - see if there's a compatible linter in devDeps or one mentioned in standard-engine. If that is the case, we start the lintWorker.

If the lintWorker cannot resolve the module it is most likely because we are in a project in which the user haven't yet run npm install. We wouldn't want that situation to require a restart of atom to pick up on a post-atom-start installed linter.

When the lintWorker is poked to lint a piece of code, we check if we have a resolved linter - if we haven't we try to resolve it again...

gustavnikolaj commented 8 years ago

At least this thing should go: https://github.com/gustavnikolaj/linter-js-standard-engine/blob/c3fdd41374118e76ee48606bc17112dd3a716335/lib/findOptions.js#L89-L92

It means that it will continue searching for a package.json file all up to the root, until it finds one that contains a linter dependency - or until it reaches the root and gives up. With the fixtures it means that we will effectively always fallback to the standard linter used in the project itself.

gustavnikolaj commented 8 years ago

The above referenced pull request made the change I suggested in my comment above.

@novemberborn could you try to elaborate here? I'm not sure that I understood the issue correctly :-)

novemberborn commented 8 years ago

It's these lines: https://github.com/gustavnikolaj/linter-js-standard-engine/blob/d55637cdfa406b0ac4e9d8236a9fe7f532164ece/lib/findOptions.js#L38:L40

We should only look at devDependencies.

gustavnikolaj commented 8 years ago

@novemberborn Good point! Doesn't make sense to have three different ways of testing for the presence. :-)