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.

Sign up for GitHub

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

Jump to bottom

Refactor collections_dir feature for consistency #6685

Merged
merged 4 commits into from Jan 25, 2018
Merged

Refactor collections_dir feature for consistency #6685

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a09b646
Refactor collections_dir feature for consistency
ashmaroli Jan 8, 2018
39d0bad
make corrections based on feedback
ashmaroli Jan 9, 2018
2af6834
props @DirtyF: handle static files in a collection
ashmaroli Jan 15, 2018
f4daf2e
compute collections_path once per build session
ashmaroli Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
156 changes: 156 additions & 0 deletions features/collections_dir.feature
View file
Open in desktop
@@ -0,0 +1,156 @@
Feature: Collections Directory
As a hacker who likes to structure content without clutter
I want to be able to organize my collections under a single directory
And render them from there

Scenario: Custom collections_dir containing only posts
And I have a collections/_posts directory
And I have the following post within the "collections" directory:
| title | date | content |
| Gathered Post | 2009-03-27 | Random Content. |
And I have a "_config.yml" file with content:
"""
collections_dir: collections
"""
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And I should see "Random Content." in "_site/2009/03/27/gathered-post.html"

Scenario: Rendered collection in custom collections_dir also containing posts
Given I have a collections/_puppies directory
And I have the following document under the "puppies" collection within the "collections" directory:
| title | date | content |
| Rover | 2007-12-31 | content for Rover. |
And I have a collections/_posts directory
And I have the following post within the "collections" directory:
| title | date | content |
| Gathered Post | 2009-03-27 | Random Content. |
And I have a "_config.yml" file with content:
"""
collections:
puppies:
output: true

collections_dir: collections
"""
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist
And I should see "Random Content." in "_site/2009/03/27/gathered-post.html"

Scenario: Rendered collection in custom collections_dir with posts at the site root
Given I have a collections/_puppies directory
And I have the following document under the "puppies" collection within the "collections" directory:
| title | date | content |
| Rover | 2007-12-31 | content for Rover. |
And I have a _posts directory
And I have the following post:
| title | date | content |
| Post At Root | 2009-03-27 | Random Content. |
And I have a "_config.yml" file with content:
"""
collections:
puppies:
output: true

collections_dir: collections
"""
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist
And the "_site/2009/03/27/post-at-root.html" file should not exist
Copy link
Member

Choose a reason for hiding this comment

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

Woof, this is going to screw people up. We can't enforce that the _posts directory is in the collections_dir until 4.0, if we even want to.

Copy link
Member Author

Choose a reason for hiding this comment

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

its not.. we just released this feature in 3.7.0 .. The sooner we ship this patch, the lesser no.of users gonna have issues..

And the _site/_posts directory should not exist

Scenario: Rendered collection in custom collections_dir also containing drafts
Given I have a collections/_puppies directory
And I have the following document under the "puppies" collection within the "collections" directory:
| title | date | content |
| Rover | 2007-12-31 | content for Rover. |
And I have a collections/_drafts directory
And I have the following draft within the "collections" directory:
| title | date | content |
| Gathered Draft | 2009-03-27 | Random Content. |
And I have a "_config.yml" file with content:
"""
collections:
puppies:
output: true

collections_dir: collections
"""
When I run jekyll build --drafts
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist
And I should see "Random Content." in "_site/2009/03/27/gathered-draft.html"
And the _site/collections directory should not exist

Scenario: Rendered collection in custom collections_dir with drafts at the site root
Given I have a collections/_puppies directory
And I have the following document under the "puppies" collection within the "collections" directory:
| title | date | content |
| Rover | 2007-12-31 | content for Rover. |
And I have a _drafts directory
And I have the following draft:
| title | date | content |
| Draft At Root | 2009-03-27 | Random Content. |
And I have a "_config.yml" file with content:
"""
collections:
puppies:
output: true

collections_dir: collections
"""
When I run jekyll build --drafts
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover.html" file should exist
And the "_site/2009/03/27/draft-at-root.html" file should not exist
Copy link
Member

Choose a reason for hiding this comment

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

_drafts can't be enforced to be in collections_dir until 4.0 either.


Scenario: A complex site with collections posts and drafts at various locations
Given I have a gathering/_puppies directory
And I have a gathering/_posts directory
And I have a gathering/_drafts directory
And I have a _puppies directory
And I have a _posts directory
And I have a _drafts directory
And I have the following document under the "puppies" collection within the "gathering" directory:
| title | date | content |
| Rover in Gathering | 2007-12-31 | content for Rover. |
And I have the following document under the puppies collection:
| title | date | content |
| Rover At Root | 2007-12-31 | content for Rover. |
And I have the following post within the "gathering" directory:
| title | date | content |
| Post in Gathering | 2009-03-27 | Totally nothing. |
And I have the following post:
| title | date | content |
| Post At Root | 2009-03-27 | Totally nothing. |
And I have the following draft within the "gathering" directory:
| title | date | content |
| Draft In Gathering | 2009-03-27 | This is a draft. |
And I have the following draft:
| title | date | content |
| Draft At Root | 2009-03-27 | This is a draft. |
And I have a "_config.yml" file with content:
"""
collections:
puppies:
output: true

collections_dir: gathering
"""
When I run jekyll build --drafts
Then I should get a zero exit status
And the _site directory should exist
And the "_site/puppies/rover-in-gathering.html" file should exist
And the "_site/2009/03/27/post-in-gathering.html" file should exist
And the "_site/2009/03/27/draft-in-gathering.html" file should exist
And the "_site/2009/03/27/draft-at-root.html" file should not exist
And the "_site/puppies/rover-at-root.html" file should not exist
And the _site/gathering directory should not exist
And the _site/_posts directory should not exist
24 changes: 24 additions & 0 deletions features/step_definitions.rb
View file
Open in desktop
Expand Up @@ -95,6 +95,20 @@

#

Given(%r!^I have the following (draft|post)s? within the "(.*)" directory:$!) do |type, folder, table|
table.hashes.each do |input_hash|
title = slug(input_hash["title"])
parsed_date = Time.xmlschema(input_hash["date"]) rescue Time.parse(input_hash["date"])

filename = type == "draft" ? "#{title}.markdown" : "#{parsed_date.strftime("%Y-%m-%d")}-#{title}.markdown"

path = File.join(folder, "_#{type}s", filename)
File.write(path, file_content_from_hash(input_hash))
end
end

#

Given(%r!^I have the following documents? under the (.*) collection:$!) do |folder, table|
table.hashes.each do |input_hash|
title = slug(input_hash["title"])
Expand All @@ -108,6 +122,16 @@

#

Given(%r!^I have the following documents? under the "(.*)" collection within the "(.*)" directory:$!) do |label, dir, table|
table.hashes.each do |input_hash|
title = slug(input_hash["title"])
path = File.join(dir, "_#{label}", "#{title}.md")
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why you chose .md for documents but .markdown for the post / drafts step above?

Copy link
Member Author

Choose a reason for hiding this comment

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

only because that's how our default site gets created from jekyll new .. and I think, an existing step also has something similar set up.. not sure..

File.write(path, file_content_from_hash(input_hash))
end
end

#

Given(%r!^I have a configuration file with "(.*)" set to "(.*)"$!) do |key, value|
config = \
if source_dir.join("_config.yml").exist?
Expand Down
11 changes: 5 additions & 6 deletions lib/jekyll/collection.rb
View file
Open in desktop
Expand Up @@ -95,14 +95,13 @@ def filtered_entries
end
end

# The directory for this Collection, relative to the site source.
# The directory for this Collection, relative to the site source or the directory
# containing the collection.
#
# Returns a String containing the directory name where the collection
# is stored on the filesystem.
def relative_directory
@relative_directory ||= Pathname.new(directory).relative_path_from(
Pathname.new(site.source)
).to_s
@relative_directory ||= "_#{label}"
Copy link
Member

Choose a reason for hiding this comment

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

This should be relative to the site source, so if source is . then it should be collections_dir/_label.

Copy link
Member Author

Choose a reason for hiding this comment

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

Things that're going to break if @relative_directory ||= collections_dir/_label:

jekyll/lib/jekyll/document.rb

Lines 32 to 36 in f77d704

if draft?
categories_from_path("_drafts")
else
categories_from_path(collection.relative_directory)
end

jekyll/lib/jekyll/document.rb

Lines 76 to 79 in f77d704

def draft?
data["draft"] ||= relative_path.index(collection.relative_directory).nil? &&
collection.label == "posts"
end

jekyll/lib/jekyll/document.rb

Lines 121 to 124 in f77d704

def cleaned_relative_path
@cleaned_relative_path ||=
relative_path[0..-extname.length - 1].sub(collection.relative_directory, "")
end

end

# The full path to the directory containing the collection.
Expand All @@ -111,7 +110,7 @@ def relative_directory
# is stored on the filesystem.
def directory
@directory ||= site.in_source_dir(
File.join(site.config["collections_dir"], "_#{label}")
File.join(site.config["collections_dir"], relative_directory)
)
end

Expand All @@ -125,7 +124,7 @@ def directory
# is stored on the filesystem.
def collection_dir(*files)
return directory if files.empty?
site.in_source_dir(relative_directory, *files)
site.in_source_dir(site.config["collections_dir"], relative_directory, *files)
end

# Checks whether the directory "exists" for this collection.
Expand Down
17 changes: 11 additions & 6 deletions lib/jekyll/document.rb
View file
Open in desktop
Expand Up @@ -83,7 +83,7 @@ def draft?
# Returns a String path which represents the relative path
# from the site source to this document
def relative_path
@relative_path ||= Pathutil.new(path).relative_path_from(site.source).to_s
@relative_path ||= Pathutil.new(path).relative_path_from(focal_point).to_s
Copy link
Member

Choose a reason for hiding this comment

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

Why can't this be the site.source? Are you thinking the collections_dir needs to disappear in the output? Hm. Maybe relative_path_from(collection.path)?

Copy link
Member Author

Choose a reason for hiding this comment

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

collection does not have an instance method :path

Copy link
Member

Choose a reason for hiding this comment

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

Any reason it can't have that method? I don't like the focal_point method – seems like it's capturing some logic that should be handled somewhere else, like path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any reason...?

I did not want to introduce a new public_method in a patch-release..

Copy link
Member

@DirtyF DirtyF Jan 21, 2018 •
edited

Choose a reason for hiding this comment

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

Well now that we have introduce a potential breaking change in a minor version, we should be above that kind of consideration here. The more collections_dir behavior is well thought, the better.

Copy link
Member Author

Choose a reason for hiding this comment

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

@parkr Your call.. should I introduce a new public_method in a patch-release..?

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 adding Collection#path to fix this bug is a fine solution for a patch release. SemVer makes no assumptions about methods being added, as far as I can tell. Our SemVer is guided by new features (minor bump) and breaking changes (major bump).

Copy link
Member Author

Choose a reason for hiding this comment

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

I would call "enhancing" Collections class with a new public_method as a feature.. just like removing an existing public_method is considered a breaking change..

That said, this is not the place for debating SemVer.. your comment above is clear enough an answer for how to proceed..
Will push a commit soon..

Copy link
Member Author

Choose a reason for hiding this comment

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

Small change of plans.. Instead of having Collection#path for each collection in a Site, I decided to compute the path once via Site#collections_path

end

# The output extension of the document.
Expand Down Expand Up @@ -399,11 +399,9 @@ def categories_from_path(special_dir)
def populate_categories
merge_data!({
"categories" => (
Array(data["categories"]) + Utils.pluralized_array_from_hash(
data,
"category",
"categories"
)
Array(data["categories"]) + Utils.pluralized_array_from_hash(
data, "category", "categories"
)
).map(&:to_s).flatten.uniq,
})
end
Expand All @@ -414,6 +412,13 @@ def populate_tags
})
end

private
def focal_point
collections_dir = site.config["collections_dir"]
@focal_point ||=
collections_dir.empty? ? site.source : site.in_source_dir(collections_dir)
end

private
def merge_categories!(other)
if other.key?("categories") && !other["categories"].nil?
Expand Down
16 changes: 16 additions & 0 deletions lib/jekyll/reader.rb
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@ def read_directories(dir = "")
#
# Returns nothing.
def retrieve_posts(dir)
return if outside_configured_directory?(dir)
site.posts.docs.concat(PostReader.new(site).read_posts(dir))
site.posts.docs.concat(PostReader.new(site).read_drafts(dir)) if site.show_drafts
end
Expand Down Expand Up @@ -130,5 +131,20 @@ def get_entries(dir, subfolder)
entries = Dir.chdir(base) { filter_entries(Dir["**/*"], base) }
entries.delete_if { |e| File.directory?(site.in_source_dir(base, e)) }
end

private

# Internal
#
# Determine if the directory is supposed to contain posts and drafts.
# If the user has defined a custom collections_dir, then attempt to read
# posts and drafts only from within that directory.
#
# Returns true if a custom collections_dir has been set but current directory lies
# outside that directory.
def outside_configured_directory?(dir)
collections_dir = site.config["collections_dir"]
!collections_dir.empty? && !dir.start_with?("/#{collections_dir}")
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of renaming this method to within_configured_directory? and then just checking with unless or if ! ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be done.. I kept it negative so that human can readily parse two negative expressions easily in comparison to one negative and the other positive..

    def within_configured_directory?(dir)
      collections_dir = site.config["collections_dir"]
      !collections_dir.empty? && dir.start_with?("/#{collections_dir}")
    end

end
end
end