Ignore paths from `.gitignore` #147

juanj · 2016-09-30T18:30:38Z

Fixes #27

sholladay · 2016-09-30T18:45:45Z

I don't think it should be behind a flag. The whole point is to reduce the burden on the user. In fact, I'd personally avoid an option until we hear of a use case for disabling it.

juanj · 2016-09-30T18:49:59Z

So will be better if the flags is to disable it?

vadimdemedes · 2016-10-01T08:40:35Z

Totally agree with @sholladay, the --use-gitignore flag is unnecessary.

vadimdemedes · 2016-10-01T08:42:02Z

@juanj Could you add a simple test, confirming it actually skips the patterns in .gitignore? I think it should be good to go then!

kevva · 2016-10-01T10:09:11Z

package.json

@@ -82,7 +82,8 @@
    "resolve-cwd": "^1.0.0",
    "resolve-from": "^2.0.0",
    "update-notifier": "^1.0.0",
-    "xo-init": "^0.3.0"
+    "xo-init": "^0.3.0",
+    "parse-gitignore": "^0.3.1"


These should be listed alphabetically.

kevva · 2016-10-01T10:11:32Z

options-manager.js

@@ -195,6 +196,10 @@ function preprocess(opts) {
 	opts = mergeWithPkgConf(opts);
 	opts = normalizeOpts(opts);
 	opts.ignores = DEFAULT_IGNORE.concat(opts.ignores || []);
+
+	const gitignore = parseGitignore('.gitignore');
+	opts.ignores = opts.ignores.concat(gitignore || []);


Create a function instead, getIgnores or something, that we then can export like we've done with the other ones.

opts.ignores = getIgnores(opts.ignores);

sindresorhus

Also needs at least one integration test, like in #148.

sindresorhus · 2016-10-03T03:59:15Z

options-manager.js

+	const gitignore = parseGitignore('.gitignore');
+
+	opts.ignores = DEFAULT_IGNORE.concat(opts.ignores || []);
+	opts.ignores = opts.ignores.concat(gitignore || []);


No need for multiple concat statement. Array#concat support multiple arguments.

sindresorhus · 2016-10-03T04:00:06Z

options-manager.js

@@ -191,10 +192,20 @@ function groupConfigs(paths, baseOptions, overrides) {
 	return arr;
 }

+function getIgnores(opts) {
+	const gitignore = parseGitignore('.gitignore');


This needs to take into account the cwd option.

sindresorhus · 2016-10-03T06:02:41Z

What if there are nested .gitignore's?

.
├── .gitignore
├── foo
│   ├── .gitignore
│   └── cake.js
└── unicorn.js

* Take in account the cwd. * Use one Array#concat * Change the test to use Array#includes * Find and use nested .gitignores

kevva · 2016-10-05T09:12:17Z

options-manager.js

+		});
+
+		ignores = ignores.concat(fullPathResult || []);
+	});


Just do the following:

const ignores = gitignores .map(path => { const res = parseGitignore(path); const location = path.substring(0, path.length - 10); return res.map(file => location + file); }) .reduce((a, b) => a.concat(b));

kevva · 2016-10-05T09:16:20Z

options-manager.js

+
+	gitignores.forEach(path => {
+		const result = parseGitignore(path);
+		const location = path.substring(0, path.length - 10);


Why do we do this? If I assume it's for removing the name from the path? Just use path.dirname() then. See https://nodejs.org/api/path.html#path_path_dirname_path.

kevva · 2016-10-05T09:19:12Z

options-manager.js

+		const fullPathResult = [];
+
+		result.forEach(file => {
+			file = location + file;


Use path.join() instead.

sindresorhus · 2016-10-07T09:04:19Z

options-manager.js

@@ -191,10 +193,27 @@ function groupConfigs(paths, baseOptions, overrides) {
 	return arr;
 }

+function getIgnores(opts) {
+	const gitignores = globby.sync('**/.gitignore');


It should only look for .gitignore files in not already ignored locations. So you need to pass the existing ignores here (in the ignore option). Right now it also looks in for example the node_modules folder, which is very slow.

Should also be a test for this (API test).

This must also take into account the XO cwd option.

sindresorhus · 2016-10-07T09:06:13Z

options-manager.js

+	const ignores = gitignores
+	.map(pathToGitignore => {
+		const result = parseGitignore(pathToGitignore);
+		const location = path.dirname(pathToGitignore);


location => base
result => patterns

sindresorhus · 2016-10-07T09:12:16Z

test/cli.js

@@ -31,6 +31,17 @@ test.failing('ignores fixture', async t => {
 	t.throws(execa('../../../cli.js', ['--no-local'], {cwd}));
 });

+test('ignore files in .gitignore', async t => {


Would be good if this test was duplicated as an API test too (using the cwd option).

Why this should be an API test?
What are the API tests for?

To make sure the cwd option is adhered to. In the CLI test it just uses process.cwd(). API tests are a lot faster to run too. Long-term, I'm going to convert many of the CLI tests to API tests.

I've merged this now, but would be nice if you could submit an API test for this.

sindresorhus · 2016-10-07T09:14:06Z

The merge conflict needs to be resolved.

sindresorhus · 2016-10-07T09:34:03Z

It also turns out parse-gitignore produces incorrect globs... node_modules in a .gitignore is equivalent to **/node_modules/**, but parse-gitignore produces node_modules/**. @juanj Can you open an issue on parse-gitignore?

See: https://git-scm.com/docs/gitignore

sindresorhus · 2016-10-10T19:04:38Z

@juanj Any chance you could finish this up? :)

sindresorhus · 2016-10-11T07:43:51Z

Looks great now. Thanks for working on this @juanj :)

Add --use-gitignore feature

81cb471

Ignore the .gitignore files by default

4d703a5

juanj changed the title ~~Add --use-gitignore feature~~ Ignore paths in.gitignore Sep 30, 2016

Inlcude description in readme

b9e5313

kevva reviewed Oct 1, 2016

View reviewed changes

juanj added 4 commits October 1, 2016 08:38

Order alphabetically.

c0eccb1

Move code to a new function

4dfe33e

Add test

13d92cb

Change ignored directory to foo

5703434

sindresorhus requested changes Oct 3, 2016

View reviewed changes

sindresorhus changed the title ~~Ignore paths in.gitignore~~ Ignore paths from .gitignore Oct 3, 2016

juanj added 3 commits October 4, 2016 08:43

Requested changes.

012c30a

* Take in account the cwd. * Use one Array#concat * Change the test to use Array#includes * Find and use nested .gitignores

Remove trailing space

57040c8

Add integration test

3566cf8

kevva reviewed Oct 5, 2016

View reviewed changes

options-manager.js

const fullPathResult = [];

result.forEach(file => {

file = location + file;

Copy link

Contributor

kevva Oct 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use path.join() instead.

juanj added 3 commits October 5, 2016 11:42

Use indexOf

1009113

Use map and reduce instead of forEach. Use path module.

2aa2776

Use path.join to generate the path to check.

9757062

sindresorhus requested changes Oct 7, 2016

View reviewed changes

juanj added 4 commits October 10, 2016 15:23

Ignore already ignored locations when searching for .gitignore

bf7d913

Take in account XO cwd option

6e0e4af

Change variable names

2f172b0

Resolve merge conflict

ce27572

sindresorhus approved these changes Oct 11, 2016

View reviewed changes

sindresorhus merged commit fa99f36 into xojs:master Oct 11, 2016

vadimdemedes mentioned this pull request Oct 11, 2016

paths in .gitignore are ignored by default #148

Closed

sindresorhus added a commit that referenced this pull request Oct 11, 2016

cleanup #147

519ad1b

snyk-bot mentioned this pull request Mar 25, 2021

[Snyk] Security upgrade meow from 3.7.0 to 8.0.0 AdminDevelopment/xo#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore paths from `.gitignore` #147

Ignore paths from `.gitignore` #147

juanj commented Sep 30, 2016 •

edited by sindresorhus

Loading

sholladay commented Sep 30, 2016

juanj commented Sep 30, 2016

vadimdemedes commented Oct 1, 2016

vadimdemedes commented Oct 1, 2016

kevva Oct 1, 2016

kevva Oct 1, 2016 •

edited

Loading

sindresorhus left a comment

sindresorhus Oct 3, 2016

sindresorhus Oct 3, 2016

sindresorhus commented Oct 3, 2016

kevva Oct 5, 2016

kevva Oct 5, 2016 •

edited

Loading

kevva Oct 5, 2016

sindresorhus Oct 7, 2016 •

edited

Loading

sindresorhus Oct 7, 2016

sindresorhus Oct 7, 2016

sindresorhus Oct 7, 2016

juanj Oct 10, 2016

sindresorhus Oct 11, 2016

sindresorhus commented Oct 7, 2016 •

edited

Loading

sindresorhus commented Oct 7, 2016

sindresorhus commented Oct 10, 2016 •

edited

Loading

sindresorhus commented Oct 11, 2016

Ignore paths from .gitignore #147

Ignore paths from .gitignore #147

Conversation

juanj commented Sep 30, 2016 • edited by sindresorhus Loading

sholladay commented Sep 30, 2016

juanj commented Sep 30, 2016

vadimdemedes commented Oct 1, 2016

vadimdemedes commented Oct 1, 2016

Choose a reason for hiding this comment

kevva Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

sindresorhus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Oct 3, 2016

Choose a reason for hiding this comment

kevva Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Oct 7, 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 commented Oct 7, 2016 • edited Loading

sindresorhus commented Oct 7, 2016

sindresorhus commented Oct 10, 2016 • edited Loading

sindresorhus commented Oct 11, 2016

Ignore paths from `.gitignore` #147

Ignore paths from `.gitignore` #147

juanj commented Sep 30, 2016 •

edited by sindresorhus

Loading

kevva Oct 1, 2016 •

edited

Loading

kevva Oct 5, 2016 •

edited

Loading

sindresorhus Oct 7, 2016 •

edited

Loading

sindresorhus commented Oct 7, 2016 •

edited

Loading

sindresorhus commented Oct 10, 2016 •

edited

Loading