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

Remove ignore paths from .json #227

Merged
merged 11 commits into from
Jan 31, 2013
Merged

Remove ignore paths from .json #227

merged 11 commits into from
Jan 31, 2013

Conversation

desandro
Copy link
Member

For #88, this adds ignore property to JSON.

This works by deleting and ignored filepaths after the package has been installed.

Tests added. Note added in the README.

I feel pretty good about this, but I'd appreciate @satazor taking a look at the code.

// 1: get paths
async.forEach(removePatterns, function (removePattern, next) {
// glob path for file path pattern matching
glob(path.join(this.localPath, removePattern), function (err, globPaths) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we must enable the dot: true option so that glob matches the .git and other files that start with a .. If it' s not actually removing the .git folder (wich I think it's not) can you add a test for it?

@satazor
Copy link
Member

satazor commented Jan 31, 2013

Looks great @desandro, please review what I've mentioned.

@desandro
Copy link
Member Author

Good call for both. Added test for .git directory, and made the push.apply change

@satazor
Copy link
Member

satazor commented Jan 31, 2013

@desandro We still have to turn on the dot option on glob (which will also activate it in the minimatch). Will quote the documentation of minimatch:

dot

Allow patterns to match filenames starting with a period, even if the pattern does not explicitly have a period in that spot.

Note that by default, a/**/b will not match a/.d/b, unless dot is set.

This means that if an user types test/**/* as the ignore pattern and there is a test/.jshintrc file, it won't be ignored unless we use dot: true.

If you add a test for this, it will probably fail.

@SBoudrias
Copy link

I don't really get the point of asking package creator to "blacklist" unnecessary files. It would be much easier for them to point to the files/directory to be kept. The other way around seems way clearer.

Plus, instead of deleting the downloaded files, you could just keep the full repo copy in the cache, then you copy "whitelisted" files over to the project.

Also, I think this should be over-writable by the end user. So let's say a user want to get twitter bootstrap, but only the modal module, he could easily just pick the file he needs.

@desandro
Copy link
Member Author

@SBoudrias I totally get your point. This seems backwards. Especially for us to implement. But using ignore is a bit friendlier to package developers. There's already a convention for ignore property. It follows same thinking with .gitignore or npm's ignore.

As for user-overwritability of ignore, let's leave that to another issue in the future.

@desandro
Copy link
Member Author

Dang, you're good. dot: true option added, which resolves the test cases for dot dirs/files.

@satazor
Copy link
Member

satazor commented Jan 31, 2013

@desandro I know that ahahah :P, joking

Thanks!

@satazor satazor closed this Jan 31, 2013
@satazor satazor reopened this Jan 31, 2013
satazor added a commit that referenced this pull request Jan 31, 2013
Remove ignore paths from .json
@satazor satazor merged commit 6195281 into master Jan 31, 2013
@satazor satazor deleted the ignore branch February 1, 2013 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants