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 linter pluggable? (PR offer) #282

Closed
smikes opened this issue Dec 12, 2014 · 23 comments
Assignees
Labels
Milestone

Comments

@smikes
Copy link
Contributor

@smikes smikes commented Dec 12, 2014

Hi!

I just tried out lab and I'm really loving it, but the standard here is to use jslint for linting. I'd be happy to write the code necessary to make the choice of linter be a pluggable option for lab.

Does that sound useful? Is there anything I should be aware of, when writing this patch? (Beyond the CONTRIBUTING and code style guidelines, which I have read..)

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Dec 13, 2014

The original feature had it, not anymore, see reason here. Guess it's time to revert the changes.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Dec 13, 2014

@smikes Sounds like a plan. Please make the -L default eslint so that the current makefiles that use linting (hapijs/hapi) don't break with this change.

@geek geek added the request label Dec 13, 2014
@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 14, 2014

Okay. I have started to implement this.

I think I will need a new option

-L     # do linting
-n     # optional, specify linter, default is eslint

or else I will need help understanding how to make Bossy handle this --

no -L      => no linting
-L         => lint with eslint
-L eslint  => lint with eslint
-L jslint  => lint with jslint

Setting the type of -L to 'string' with default 'eslint' would turn linting on by default, but setting the default to null would mean that -L (no argument) would have no linting, both of which are undesirable.

So I think adding a new option (-n) that only people who want a non-standard linter care need to worry about is the right way to go.. thoughts?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Dec 14, 2014

This should be fixed in bossy, not worked around with another switch.

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 14, 2014

OK, created hapijs/bossy#30 to add a way to specify this behavior in bossy.

Comments very welcome, as the logic gets a bit complicated.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Dec 14, 2014

I think you'll need to add a test for multiple options (eg. '-L -f', '-Lf', '-L eslint -f', ...).
That's probably not on you but I think there's already a bug in bossy with such input.

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 14, 2014

Sure, let's discus that in hapijs/bossy#30 or another issue over there.

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 14, 2014

@Marsup I think you're right, though, there is a backwards-compatibility problem with giving an argument to -L. When -L is a no-argument boolean switch, it can be combined with other such switches in any order => -fL === -Lf === -L -f

But when -L takes an optional string argument, -fL === -f -L but -Lf === -L f which fails to validate. And -fL should ==> -f -L but this currently gives an error Invalid option: lint missing value, which is not what I expected.

@arb

This comment has been minimized.

Copy link
Contributor

@arb arb commented Dec 14, 2014

The change to bossy to accommodate this seems overly complicated. Why not just add one or two conditional statements in lab to achieve this as you mentioned before. I'm not a huge fan of -L really meaning -L eslint. Seems kind like a cryptic feature where the presence of a flag is almost always used for boolean values.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Dec 15, 2014

It's a 10 lines patch, what's so complicated about that ? Many unix tools have this kind of switch.

@arb

This comment has been minimized.

Copy link
Contributor

@arb arb commented Dec 15, 2014

If we weren't trying to keep the current -L behavior, this wouldn't even be an issue - this all just seems like a lot of dancing just to maintain backward compatibility.

I guess if other linux tools do this then I guess I can live with it.

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 16, 2014

I'd be okay with using an environment variable or some other method (other than a flag) for specifying the linter.

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 16, 2014

reverted to -L means lint, -n optionally specifies lint program, per comments in hapijs/bossy#30

@kemitchell

This comment has been minimized.

Copy link

@kemitchell kemitchell commented Dec 16, 2014

I found this issue when I used a 0.10.x eslint rule and got an error from bundled 0.8.x.

Would -n eslint shell out to eslint or use a version packaged with lab?

@smikes

This comment has been minimized.

Copy link
Contributor Author

@smikes smikes commented Dec 16, 2014

For this issue I plan to leave the eslint code alone, so "-L -n eslint" would behave identically to "-L", ie use the bundled eslint.

On Tue, Dec 16, 2014 at 4:18 PM, Kyle Mitchell notifications@github.com
wrote:

I found this issue when I used a 0.10.x eslint rule and got an error from bundled 0.8.x.

Would -n eslint shell out to eslint or use a version packaged with lab?

Reply to this email directly or view it on GitHub:
#282 (comment)

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Dec 16, 2014

At some point that might be interesting to separate things though, like code has been. I still don't understand your reluctance to use a single switch, maybe I missed something...

@tjmehta

This comment has been minimized.

Copy link
Contributor

@tjmehta tjmehta commented Oct 10, 2015

+1, how can I support alternative linters like standard

@cjihrig

This comment has been minimized.

Copy link
Contributor

@cjihrig cjihrig commented Oct 11, 2015

standard isn't a linter. It's a config file and some other tooling built on top of ESLint. You can specify the standard ESLint file through Lab. Or just install standard and add it to your package.json or whatever you use.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 12, 2015

@tjmehta add .eslintrc file to your project with the following contents:

{
  "extends": "standard"
}

next you just need to install a couple of packages: "eslint-config-standard", "eslint-plugin-standard"

@tjmehta

This comment has been minimized.

Copy link
Contributor

@tjmehta tjmehta commented Feb 16, 2016

Looks like eslint@^1.10.0 is currently bundled w/ lab, can lab use an external eslint?

Specific Problem: eslint-config-standard now requires eslint@^2.0.0. Am I stuck using an old eslint-config that is compatible with eslint@1?

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

@gergoerdosi gergoerdosi commented Feb 17, 2016

I was about to send a pull request this week for eslint 2.0.0, because we need it too. If that change gets published with a new major version of lab, then our problem gets solved. Of course pluggable linters would be the best, but I assume it would require a lot more work.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Feb 17, 2016

@tjmehta and @gergoerdosi we will update to eslint 2, I also want to make linters pluggable so that I can pull out jshint from being bundled.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Feb 17, 2016

see #519

@geek geek added breaking changes and removed request labels Feb 18, 2016
@geek geek added this to the 9.0.0 milestone Feb 18, 2016
@geek geek self-assigned this Feb 18, 2016
@geek geek added feature and removed breaking changes labels Feb 18, 2016
@geek geek closed this in #522 Feb 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.