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

WIP - Add linting support #213

Merged
merged 1 commit into from Oct 6, 2014

Conversation

@Marsup
Copy link
Member

Marsup commented Sep 19, 2014

Hello,

As discussed in outmoded/discuss#22, I'm trying to implement linting in lab.
This is a WIP and shouldn't be merged right now as tests are not even done.
So far I only have support for eslint, I included @geek's rules but lab itself is full of errors so it'll need tweaking.
It will automatically detect eslint settings at the root of the project and if none use what should be our default rules.
The possibility to have multiple linters is deliberate and probably wanted.
Console output was faster to implement but others will follow.
I'd like feedback about my approach.

Thanks.

@Marsup Marsup force-pushed the Marsup:lint branch from c23dd8c to aa420a3 Sep 19, 2014
@hueniverse hueniverse added the feature label Sep 19, 2014
bin/_lab Outdated
var ChildProcess = require('child_process');
if (process.env.ROOT_SPAWN) {
require('../test_runner/cli').run();
} else {

This comment has been minimized.

Copy link
@geek

geek Sep 24, 2014

Member

else on newline

lib/lint.js Outdated

linters.forEach(function (linter) {

if (linter === 'eslint') {

This comment has been minimized.

Copy link
@geek

geek Sep 24, 2014

Member

Might be simpler to check:

Hoek.assert(internals.linters.indexOf(linter) !== -1, 'Linter ' + linter + ' is unknown.');

This comment has been minimized.

Copy link
@Marsup

Marsup Sep 25, 2014

Author Member

@geek Indeed but there's no hoek so far, is it worth depending on it only for this ?

This comment has been minimized.

Copy link
@geek

geek Oct 2, 2014

Member

I think it makes sense, its a consistent way to assert on settings issues.

@geek

This comment has been minimized.

Copy link
Member

geek commented Sep 24, 2014

I like this approach, be sure to add more tests and document the option in the readme.

@Marsup Marsup force-pushed the Marsup:lint branch from aa420a3 to b235885 Oct 2, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Oct 2, 2014

Updated the PR with these suggestions. I've actually spent a lot of time trying to find the correct settings to validate lab itself. If you have better rules before starting writing our own, I'll take em :)

@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 2, 2014

@Marsup thanks, I'll merge this after 4.5.2. Maybe we should start with only a few rules?

@geek geek added this to the 4.6.0 milestone Oct 2, 2014
@geek geek self-assigned this Oct 2, 2014
@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 2, 2014

@Marsup any chance that you can add some tests for the new option?

@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Oct 2, 2014

Sure, I'll hopefully complete coverage in a few days, just a bit busy currently.

@Marsup Marsup force-pushed the Marsup:lint branch from b235885 to 52008c4 Oct 4, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Oct 5, 2014

@geek Coverage is good now, linting doesn't make a test run fail though, but at least it's usable until we manage to find rules that fit the projects. Travis's output is weird, a red cross but no error, I don't get it.

@Marsup Marsup force-pushed the Marsup:lint branch from 52008c4 to 5d43b4c Oct 5, 2014
geek added a commit that referenced this pull request Oct 6, 2014
WIP - Add linting support
@geek geek merged commit d955c60 into hapijs:master Oct 6, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented Oct 6, 2014

Thanks for this, it will be a huge help

@Marsup Marsup deleted the Marsup:lint branch Oct 7, 2014
mdarnall added a commit to mdarnall/joi that referenced this pull request Jan 21, 2015
* based on last activity on Lab (hapijs/lab#213)
* make all rules that are best practices, but not in styleguide as
warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.