feat: apply `.ignores` to `lintText` #169

marionebl · 2016-11-26T22:53:30Z

add multimatch-powered short-circuit to lintText
add supporting test cases
throw for opts.ignores without opts.filename
use default ignores when opts.filename is given
closes lintText does not apply ignores config #168

BREAKING CHANGES:

lintText now uses the same default ignores as lintFiles with opts.filename
- /node_modules/
- /bower_components/
- coverage/**
- {tmp,temp}/**
- **/*.min.js
- **/bundle.js
- fixtures{-*,}.{js,jsx}
- fixture{s,}/**
- {test,tests,spec,tests}/fixture{s,}/**
- vendor/**
- dist/**
lintText now throws when providing opts.ignores without opts.filename

- add multimatch-powered short-circuit to `lintText` - add supporting test cases - closes #168

sindresorhus · 2016-11-27T07:01:58Z

index.js

+				}]
+			};
+		}
+	}


This needs to take into account the overrides logic and should be after the below overrides logic. Right now it wouldn't work if you used config overrides that enabled it again. Should also have a test for that to ensure it works.

Done. Amounts to another breaking change as the default ignores now take effect

Test: https://github.com/sindresorhus/xo/pull/169/files#diff-9265a69d5e81bfaab4edbd417a33a626R49

sindresorhus · 2016-11-27T07:03:29Z

index.js

 const optionsManager = require('./options-manager');

 exports.lintText = (str, opts) => {
 	opts = optionsManager.preprocess(opts);

+	if (opts.cwd && opts.filename) {


To be user-friendly, I think we should throw an error if the user of lintText tries to specify the ignore option, but didn't supply cwd and filename.

Done! Guess this is a breaking change as passing ignores before these changes did nothing.

* merge applicable overrides before ignoring files * throw for opts.ignores without opts.filename * use default ignores when opts.filename is given * add test case for default ignores BREAKING CHANGES: * lintText now uses the same default ignores as lintFiles with opts.filename * **/node_modules/** * **/bower_components/** * coverage/** * {tmp,temp}/** * **/*.min.js * **/bundle.js * fixtures{-*,}.{js,jsx} * fixture{s,}/** * {test,tests,spec,__tests__}/fixture{s,}/** * vendor/** * dist/** * lintText now throws when providing opts.ignores without opts.filename

marionebl · 2016-11-27T16:33:03Z

test/api.js

+	t.is(result.warningCount, 0);
+});
+
+test('.lintText() - respect overrides', t => {


As requested: https://github.com/sindresorhus/xo/pull/169/files#r89684310

sindresorhus · 2016-11-29T05:04:09Z

index.js

 		const foundOverrides = optionsManager.findApplicableOverrides(filename, overrides);
 		opts = optionsManager.mergeApplicableOverrides(opts, foundOverrides.applicable);
 	}

 	opts = optionsManager.buildConfig(opts);
+	const defaultIgnores = optionsManager.getIgnores({}).ignores;
+
+	if (opts.ignores && !arrayEqual(defaultIgnores, opts.ignores) && typeof opts.filename !== 'string') {


I don't really understand the purpose of the !arrayEqual(defaultIgnores, opts.ignores) check. ?

I would also prefer using lodash.isequal instead as it's more general and could be used for other deep checks.

array-equal => lodash.isequal

the check is there to test if the user provided ignores via .ignores or .overrides[index].ignores. opts.ignores is always defined

sindresorhus · 2016-11-29T05:06:22Z

index.js

+	const defaultIgnores = optionsManager.getIgnores({}).ignores;
+
+	if (opts.ignores && !arrayEqual(defaultIgnores, opts.ignores) && typeof opts.filename !== 'string') {
+		throw new Error(`xo: opts.filename must be string when providing opts.ignores. Received value "${opts.filename}" of type "${typeof opts.filename}" for opts.filename.`);


This is a bit too verbose. People will most likely never hit this as we're the only ones using the API. I also don't see the point of the xo: prefix. It's clear from the stack trace where the error is coming from.

I'd prefer something like:

> The `ignores` option requires the `filename` option to be defined.

sindresorhus · 2016-11-29T05:09:47Z

test/api.js

+		overrides: [
+			{
+				files: ['ignored/**/*.js'],
+				ignores: []


In addition, can you also do an overrides test that has overridden ignores?

marionebl · 2016-11-29T18:17:52Z

@sindresorhus Could you have another look?

sindresorhus · 2016-11-29T18:24:24Z

the check is there to test if the user provided ignores via .ignores or .overrides[index].ignores. opts.ignores is always defined

Feels a bit overkill. How about just putting a variable at the top with:

const hasIgnores = Array.isArray(opts.ignores);

And using that further down.

marionebl · 2016-11-29T18:25:10Z

Feels a bit overkill. How about just putting a variable at the top with:

const hasIgnores = Array.isArray(opts.ignores);
And using that further down.

That would not handle ignores in opts.overrides then. We probably could check on both places but I thought the current solution to be more elegant. Change accordingly?

sindresorhus · 2016-11-29T18:39:21Z

Right, forgot about the ignores in overrides. I'm still not getting how !isEqual(defaultIgnores, opts.ignores) works though (might be because it's late here and I'm half asleep). Wouldn't it have a false-positive if both defaultIgnores were opts.ignores empty arrays?

marionebl · 2016-11-29T21:48:58Z

Not sure if the current approach is good enough if it is hard to grasp. Let's defer this until tomorrow and have some sleep 💤

marionebl · 2016-11-30T17:48:17Z

Coming back to this: Given there was a case with two empty array this would do the right thing and skip the error-producing code, right?

marionebl · 2016-12-13T03:47:57Z

@sindresorhus Can we move forward?

sindresorhus · 2016-12-13T12:19:33Z

Yes, sorry about the delay. Looks great. Thank you :)

feat: apply .ignores to lintText

3910206

- add multimatch-powered short-circuit to `lintText` - add supporting test cases - closes #168

sindresorhus requested changes Nov 27, 2016

View reviewed changes

marionebl force-pushed the feat/apply-ignores-lint-text branch from 182f941 to cbead57 Compare November 27, 2016 08:06

marionebl commented Nov 27, 2016

View reviewed changes

sindresorhus reviewed Nov 29, 2016

View reviewed changes

marionebl added 3 commits November 29, 2016 12:43

style: use lodash.isequal, shorten error message

e7182ef

test: add case with overriden ignores

9ce92bb

chore: remove array-equal

16ba375

sindresorhus merged commit 2e14d1b into master Dec 13, 2016

sindresorhus deleted the feat/apply-ignores-lint-text branch December 13, 2016 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: apply `.ignores` to `lintText` #169

feat: apply `.ignores` to `lintText` #169

marionebl commented Nov 26, 2016 •

edited

Loading

sindresorhus Nov 27, 2016

marionebl Nov 27, 2016

marionebl Nov 27, 2016

sindresorhus Nov 27, 2016

marionebl Nov 27, 2016

marionebl Nov 27, 2016

sindresorhus Nov 29, 2016 •

edited

Loading

marionebl Nov 29, 2016

sindresorhus Nov 29, 2016 •

edited

Loading

sindresorhus Nov 29, 2016

marionebl Nov 29, 2016

marionebl commented Nov 29, 2016

sindresorhus commented Nov 29, 2016

marionebl commented Nov 29, 2016 •

edited

Loading

sindresorhus commented Nov 29, 2016

marionebl commented Nov 29, 2016

marionebl commented Nov 30, 2016

marionebl commented Dec 13, 2016

sindresorhus commented Dec 13, 2016

feat: apply .ignores to lintText #169

feat: apply .ignores to lintText #169

Conversation

marionebl commented Nov 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marionebl commented Nov 29, 2016

sindresorhus commented Nov 29, 2016

marionebl commented Nov 29, 2016 • edited Loading

sindresorhus commented Nov 29, 2016

marionebl commented Nov 29, 2016

marionebl commented Nov 30, 2016

marionebl commented Dec 13, 2016

sindresorhus commented Dec 13, 2016

feat: apply `.ignores` to `lintText` #169

feat: apply `.ignores` to `lintText` #169

marionebl commented Nov 26, 2016 •

edited

Loading

sindresorhus Nov 29, 2016 •

edited

Loading

sindresorhus Nov 29, 2016 •

edited

Loading

marionebl commented Nov 29, 2016 •

edited

Loading