search

gustavnikolaj / linter-js-standard-engine

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

configurable linter #15

Closed gustavnikolaj closed 8 years ago

gustavnikolaj commented 8 years ago

Following up on @novemberborn's awesome work in #14

novemberborn commented 8 years ago

I've pushed some commits to update the README.

I'm thinking maybe the linter resolution should happen in the worker, rather than in Atom. I suspect that may work better if global dependencies are assumed. We'd need better communication of when the linter cannot be resolved though.

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. Or alternatively to detect one or more possible linters and have the worker find out the first one that is actually installed. We could make these changes outside of this PR though.

gustavnikolaj commented 8 years ago

Great stuff!

I'm thinking maybe the linter resolution should happen in the worker, rather than in Atom. I suspect that may work better if global dependencies are assumed. We'd need better communication of when the linter cannot be resolved though.

It might be obvious, but just to make sure that we have a common understanding of this: The reason I created the lint worker concept was to avoid instantiating a new linter for every run. Once the linter is started, it can be reused to lint different files with the same configuration with no problem.

As long as we can still maintain that semantic (maybe by keying by the arguments which we use to resolve, instead of the resolved data itself) I'm fine with moving that - or if we can proof that it was a premature optimization. I'm pretty sure that I tested this stuff, and that part of the reason I started writing this plugin, was that I found the performance too bad on the existing plugin.

I created #18 to follow up on that separately.

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. Or alternatively to detect one or more possible linters and have the worker find out the first one that is actually installed. We could make these changes outside of this PR though.

Personally I think it would be completely OK if we didn't do anything at all, unless you had a supported linter in devDependencies, or a standard-engine compliant linter in the standard-engine property in package.json.

The odds of being helpful are no better than the odds of being annoying, when we have no clue about the preference in the code we're presented with.

I'm not completely sure that I understood what you said - so please elaborate if I've gone completely off the road :-)

I think it's a good idea to get this landed as soon as possible, and do the followup changes separately, as you proposed, so I've created issues for the remaining stuff that I found. Please chip in if I missed something.

It's been a while since I worked with Atom plugins last, so I don't remember if there's anything else I have to do, than adding you as a collaborator on github, to make you able to publish new versions? I can land and publish this one but for the future it wouldn't make sense to block new releases on me being available to publish them.

gustavnikolaj commented 8 years ago

I'll merge, tag and publish this as soon as you give the green light. :-)

novemberborn commented 8 years ago

It might be obvious, but just to make sure that we have a common understanding of this: The reason I created the lint worker concept was to avoid instantiating a new linter for every run. Once the linter is started, it can be reused to lint different files with the same configuration with no problem.

Yup, that was my understanding.

Personally I think it would be completely OK if we didn't do anything at all, unless you had a supported linter in devDependencies, or a standard-engine compliant linter in the standard-engine property in package.json.

The odds of being helpful are no better than the odds of being annoying, when we have no clue about the preference in the code we're presented with.

Yea. So let's change the resolution logic to only inspect devDependencies and the standard-engine config. We can take the latter at face value, provided of course the worker can resolve it in #18.

I think it's a good idea to get this landed as soon as possible, and do the followup changes separately, as you proposed, so I've created issues for the remaining stuff that I found. Please chip in if I missed something.

Agreed. Will open an issue for the resolution logic.

It's been a while since I worked with Atom plugins last, so I don't remember if there's anything else I have to do, than adding you as a collaborator on github, to make you able to publish new versions? I can land and publish this one but for the future it wouldn't make sense to block new releases on me being available to publish them.

Did a quick search but it's not clear to me either.

I'll merge, tag and publish this as soon as you give the green light. :-)

🍏 📗 💚

novemberborn commented 8 years ago

Going by https://github.com/atom/apm/issues/284 I suspect I have publish access.

gustavnikolaj commented 8 years ago

Will release to apm as soon as I have the CI passing for both travis and appveyor :-)

gustavnikolaj commented 8 years ago

Released as 0.1.0 (https://github.com/gustavnikolaj/linter-js-standard-engine/commit/692d24558042d4502a557d345549895f690585b3) saving the big 1.0.0 to some of the other offspring issues have been fixed :-)