Skip to content

Scope path glob - #6268

Merged
jekyllbot merged 3 commits into
jekyll:masterfrom
brainbeanapps:scope_path_glob
Oct 9, 2017
Merged

Scope path glob#6268
jekyllbot merged 3 commits into
jekyll:masterfrom
brainbeanapps:scope_path_glob

Conversation

@alexey-pelykh

Copy link
Copy Markdown
Contributor

Same as #5854

When this may be useful: when you have two different set of files (e.g. *.doc & *.pdf) in the same folder that requires different values used by Jekyll plugins or Jekyll itself.
Examples:

defaults:
  # Content
  -
    scope:
      path: "content/*.pdf"
    values:
      sitemap: false
  -
    scope:
      path: "content/*.html"
    values:
      sitemap: true

or

defaults:
  # Offers
  -
    scope:
      path: "offers/*/index.html"
    values:
      layout: offer
  -
    scope:
      path: "offers/*/thank-you.html"
    values:
      sitemap: false

@site = fixture_site({
"defaults" => [{
"scope" => {
"path" => "contacts/*.html",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if I have *.html here? Should it apply to about.html and contacts/index.html?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we're using Dir.glob, according to docs, *.html will match only files on top directory, to match any HTML file, a **/*.html pattern should we used

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's what would happen with the current code, yeah. I'm wondering if it makes sense to have *.html apply to all files in my entire source that end in .html. If I were using globs, that's what I would expect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems to me that EntryFilter#glob_include behaves in a similar way since File.fnmatch uses same rules of matching. Making *.html to match all files recursively will limit usability of the feature since if I want to set values only for root html files for whatever reason and not for any other - it will be impossible to achieve. Anyhow, this behaviour should be documented somewhere?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should definitely be documented! If you wanted to apply to just root, would /*.html work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, it would, though it would definitely take more custom logic that using Dir#glob and then there would be question why EntryFilter treats *.html differently

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding the documentation - where should I write what?

site_path = Pathname.new(@site.source)
rel_scope_path = Pathname.new(scope["path"])
abs_scope_path = File.join(@site.source, rel_scope_path)
Dir.glob(abs_scope_path).each do |scope_path|

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have some code in EntryFilter#glob_include? we might be able to use here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looked through that code, it relies on File#fnmatch? that uses the same rules as Dir#glob, though EntryFilter#glob_include? itself is not super-useful in this code, it would take more lines of code to reuse glob_include?

@parkr parkr self-assigned this Aug 4, 2017
@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@parkr what's next?

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@parkr rebased

@DirtyF

DirtyF commented Sep 25, 2017

Copy link
Copy Markdown
Member

@alexey-pelykh That's great, could you add a usage example in our documentation?

Note that CI is failing because of ABC/Metrics Cop. If you can refactor it's great, if not just add the offended file in .rubocop.yml

@ashmaroli

Copy link
Copy Markdown
Member

If you can refactor it's great, if not just add the offended file in .rubocop.yml

Do not add the file to .rubocop.yml.
Instead refactor the two Pathname.new(sanitized_path).ascend { |path| [...] } into a private method..

That should appease Rubocop..
Also the last return isn't necessary.. Ruby always returns the last statement evaluated..

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DirtyF to which section should I add that example?
@ashmaroli hope it will work better now

path_is_subpath?(sanitized_path, rel_scope_path)
end

def path_is_subpath?(path, parent_path)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you want to expose this method as a public method? or would it rather be a private helper method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ashmaroli Isn't there a private scope starting right before applies?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

right.. my bad.. ignore..

@alexey-pelykh

Copy link
Copy Markdown
Contributor Author

@DirtyF docs and example added

@DirtyF
DirtyF requested a review from a team September 26, 2017 09:40

@parkr parkr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️

@DirtyF

DirtyF commented Oct 9, 2017

Copy link
Copy Markdown
Member

@jekyllbot: merge +minor

@letrastudio

Copy link
Copy Markdown

Is it possible to attach this to a release milestone yet?

Found it mentioned in the documentation (it was exactly what I was looking for!), sad to see it's unreleased. I can provide my specific usecase if it helps.

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 8, 2017
@ashmaroli

Copy link
Copy Markdown
Member

@letrastudio this has already been merged to the master branch. It'll be included in the next release. If you want, you can use it right now by pointing your Gemfile to the master branch:

gem "jekyll", :git => "https://github.com/jekyll/jekyll.git"

@jekyll jekyll locked and limited conversation to collaborators Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants