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 node_modules #11

Merged
merged 2 commits into from
Mar 20, 2015
Merged

ignore node_modules #11

merged 2 commits into from
Mar 20, 2015

Conversation

haoxins
Copy link
Contributor

@haoxins haoxins commented Mar 8, 2015

No description provided.

@haoxins
Copy link
Contributor Author

haoxins commented Mar 8, 2015

the case is requireDir with recurse: true, and there are node_modules exist.

@haoxins
Copy link
Contributor Author

haoxins commented Mar 9, 2015

ping @aseemk

@haoxins
Copy link
Contributor Author

haoxins commented Mar 19, 2015

@aseemk any advice?

@aseemk
Copy link
Owner

aseemk commented Mar 19, 2015

Sorry for missing this! Will TAL.

@aseemk
Copy link
Owner

aseemk commented Mar 19, 2015

Thanks for doing this!

My only major piece of feedback is that instead of handling this at the top of the requireDir function, we should instead handle it where we recurse. i.e. Don't recurse if base === 'node_modules'.

(Note that base has already been derived there too.)

The advantage of that is it'll support an explicit requireDir('./node_modules'), and it won't return an empty node_modules entry if we're skipping it.

It'd also be nice if there were a test for this, and also if the readme were updated to mention this. I don't mind doing these things, but might not be able to get to it right away.

Finally, ideally, this would be an option, just set to the default. But perhaps it makes sense to do a more general ignore option instead, so I don't mind not worrying about the option for now.

@haoxins
Copy link
Contributor Author

haoxins commented Mar 19, 2015

@aseemk done

ci failed on [email protected]

how about rm 0.8 and add 0.12, iojs?

@@ -2,4 +2,5 @@ language: node_js
node_js:
- "0.11"
- "0.10"
- "0.8"
- "0.12"
- "iojs"
Copy link
Owner

Choose a reason for hiding this comment

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

👍 Let's remove 0.11 while we're at it.

@aseemk
Copy link
Owner

aseemk commented Mar 20, 2015

Great work, and thanks! Happy to merge after these final changes.

@haoxins
Copy link
Contributor Author

haoxins commented Mar 20, 2015

everything should be OK 😄

aseemk added a commit that referenced this pull request Mar 20, 2015
@aseemk aseemk merged commit a74bc77 into aseemk:master Mar 20, 2015
@aseemk
Copy link
Owner

aseemk commented Mar 20, 2015

Published to npm as v0.2.0. Thanks again!

@haoxins haoxins deleted the ignore-node_modules branch March 20, 2015 14:41
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.

2 participants