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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be much more specific about ignoring specific vendored directories. #5564

Merged
merged 2 commits into from Nov 10, 2016

Conversation

Projects
None yet
6 participants
@parkr
Member

parkr commented Nov 9, 2016

An alternative to github/pages-gem#352, but it probably won't work.

The way exclusion works in Jekyll is really pretty na茂ve. In EntryFilter#filter, it iterates through an array of pathnames and checks to see if each pathname is included, excluded, etc. The way reading works in Jekyll, this list of pathnames is just the basename, so vendor/cache becomes first vendor, then (if not excluded), we get the array of files and directories inside of vendor as basenames, e.g. cache instead of vendor/cache.

I'd love to rethink the way we do reading so we get the full pathname from the site source, or allow EntryFilter's base_directory path to be included in exclusion testing.

I checked and the code for exclusion already does this! 馃帀

def excluded?(entry)
  glob_include?(site.exclude, relative_to_source(entry))
end
Show outdated Hide outdated lib/jekyll/configuration.rb
@@ -17,7 +17,7 @@ class Configuration < Hash
# Handling Reading
"safe" => false,
"include" => [".htaccess"],
"exclude" => %w(node_modules vendor),
"exclude" => %w(node_modules vendor/bundle vendor/ruby vendor/cache),

This comment has been minimized.

@ashmaroli

ashmaroli Nov 9, 2016

Member

is there a way to use wildcards to exclude any future sub-directory under /vendor/?

@ashmaroli

ashmaroli Nov 9, 2016

Member

is there a way to use wildcards to exclude any future sub-directory under /vendor/?

This comment has been minimized.

@parkr

parkr Nov 9, 2016

Member

@ashmaroli You'd just exclude vendor/. In this case, excluding all of vendor/ has caused a lot of problems so we're reverting it.

@parkr

parkr Nov 9, 2016

Member

@ashmaroli You'd just exclude vendor/. In this case, excluding all of vendor/ has caused a lot of problems so we're reverting it.

This comment has been minimized.

@ashmaroli

ashmaroli Nov 9, 2016

Member

nevermind.. i jus saw the PR body of the linked pull and your explanation...

@ashmaroli

ashmaroli Nov 9, 2016

Member

nevermind.. i jus saw the PR body of the linked pull and your explanation...

@parkr

This comment has been minimized.

Show comment
Hide comment

@parkr parkr added the friction label Nov 9, 2016

parkr added some commits Nov 9, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@crispgm

This comment has been minimized.

Show comment
Hide comment
@crispgm

crispgm Nov 10, 2016

Member

What about making the exclusion more explicitly?

Member

crispgm commented Nov 10, 2016

What about making the exclusion more explicitly?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 10, 2016

Member

What about making the exclusion more explicitly?

@crispgm What do you mean? We are making the exclusion more explicit here. By adding the /, then we are saying "ignore these directories." We don't want to exclude all of vendor/.

Member

parkr commented Nov 10, 2016

What about making the exclusion more explicitly?

@crispgm What do you mean? We are making the exclusion more explicit here. By adding the /, then we are saying "ignore these directories." We don't want to exclude all of vendor/.

@parkr parkr changed the title from Be much more specific about ignoring vendored directories. to Be much more specific about ignoring specific vendored directories. Nov 10, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 10, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Nov 10, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit c7b3017 into master Nov 10, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Nov 10, 2016

@jekyllbot jekyllbot deleted the vendor-bundle branch Nov 10, 2016

jekyllbot added a commit that referenced this pull request Nov 10, 2016

@ashmaroli ashmaroli referenced this pull request Nov 15, 2016

Merged

Exclude vendor by default #5361

1 of 1 task complete

@DirtyF DirtyF referenced this pull request Jun 9, 2017

Closed

Can't build new site. #6127

5 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment