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

Ignore paths from .gitignore #147

Merged
merged 17 commits into from
Oct 11, 2016
Merged

Ignore paths from .gitignore #147

merged 17 commits into from
Oct 11, 2016

Conversation

juanj
Copy link
Contributor

@juanj juanj commented Sep 30, 2016

Fixes #27

@sholladay
Copy link

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
Copy link
Contributor Author

juanj commented Sep 30, 2016

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

@juanj juanj changed the title Add --use-gitignore feature Ignore paths in.gitignore Sep 30, 2016
@vadimdemedes
Copy link

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

@vadimdemedes
Copy link

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

@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be listed alphabetically.

@@ -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 || []);
Copy link
Contributor

@kevva kevva Oct 1, 2016

Choose a reason for hiding this comment

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

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);

Copy link
Member

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

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

const gitignore = parseGitignore('.gitignore');

opts.ignores = DEFAULT_IGNORE.concat(opts.ignores || []);
opts.ignores = opts.ignores.concat(gitignore || []);
Copy link
Member

Choose a reason for hiding this comment

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

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

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

function getIgnores(opts) {
const gitignore = parseGitignore('.gitignore');
Copy link
Member

Choose a reason for hiding this comment

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

This needs to take into account the cwd option.

@sindresorhus sindresorhus changed the title Ignore paths in.gitignore Ignore paths from .gitignore Oct 3, 2016
@sindresorhus
Copy link
Member

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
});

ignores = ignores.concat(fullPathResult || []);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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));


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

@kevva kevva Oct 5, 2016

Choose a reason for hiding this comment

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

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.

const fullPathResult = [];

result.forEach(file => {
file = location + file;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use path.join() instead.

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

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

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

This must also take into account the XO cwd option.

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

Choose a reason for hiding this comment

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

location => base
result => patterns

@@ -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 => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

sindresorhus commented Oct 7, 2016

The merge conflict needs to be resolved.

@sindresorhus
Copy link
Member

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
Copy link
Member

sindresorhus commented Oct 10, 2016

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

@sindresorhus sindresorhus merged commit fa99f36 into xojs:master Oct 11, 2016
@sindresorhus
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants