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

Parse nested gitignores correctly #203

Merged
merged 4 commits into from Apr 30, 2017

Conversation

marionebl
Copy link
Contributor

@marionebl marionebl changed the title Fix/parse nested gitignores correctly fix: parse nested gitignores correctly Apr 2, 2017
@marionebl marionebl force-pushed the fix/parse-nested-gitignores-correctly branch from 85b3298 to d24a0cd Compare April 3, 2017 17:01
@sindresorhus sindresorhus changed the title fix: parse nested gitignores correctly Parse nested gitignores correctly Apr 19, 2017
@sindresorhus
Copy link
Member

Looks good to me.

@marionebl Can you rebase on master?

@schnittstabil Looks good to you?

@marionebl marionebl force-pushed the fix/parse-nested-gitignores-correctly branch from d24a0cd to 437f425 Compare April 22, 2017 19:08
@@ -6,21 +6,26 @@ import execa from 'execa';

process.chdir(__dirname);

const cli = (...args) => {
const cliPath = path.join(__dirname, '../cli.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor: this can be done outside of the cli function

Copy link
Member

Choose a reason for hiding this comment

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

Nah. It's not used outside the function, so better to keep it in context and it's doesn't really affect performance to keep it there.

@schnittstabil
Copy link
Contributor

Sorry, I dunno why I've missed this PR. I'll try to review it at this weekend.

Copy link
Contributor

@schnittstabil schnittstabil left a comment

Choose a reason for hiding this comment

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

Mostly minor changes.

.sync('**/.gitignore', {
ignore: ignores,
cwd: cwd
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small suggestion:

 const getGitIgnores = opts => {
-       const ignores = opts.ignores || [];
+       const ignore = opts.ignores || [];
        const cwd = opts.cwd || process.cwd();
 
        return globby
-               .sync('**/.gitignore', {
-                       ignore: ignores,
-                       cwd: cwd
-               })
+               .sync('**/.gitignore', {ignore, cwd})
                .map(filename => {
                        const fullFilename = path.join(cwd, filename);
                        const patterns = parseGitignore(fullFilename);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


t.deepEqual(result, expected);
process.chdir(previous);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the test description, I believe the following makes more sense:

 test('patterns should be translated according to process.cwd()', t => {
        const previous = process.cwd();
-       const cwd = path.join(__dirname, 'fixtures/gitignore/test');
+       const cwd = path.join(__dirname, 'fixtures/gitignore');
        process.chdir(cwd);
        const result = manager.getGitIgnores({});
-       const expected = ['!foo.js', '!foo.js/**'];
+       const expected = ['!test/foo.js', '!test/foo.js/**'];
 
        t.deepEqual(result, expected);
        process.chdir(previous);
 });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@marionebl marionebl force-pushed the fix/parse-nested-gitignores-correctly branch from f166b0f to 30d4524 Compare April 30, 2017 15:55
@schnittstabil
Copy link
Contributor

👍 LGTM

@marionebl marionebl merged commit d7ce41e into master Apr 30, 2017
@sindresorhus sindresorhus deleted the fix/parse-nested-gitignores-correctly branch April 30, 2017 18:24
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

3 participants