Refactor collections_dir feature for consistency #6685

ashmaroli · 2018-01-08T12:05:57Z

I believe this removes bugs with the recently released collection_dir feature reported via jekyll/jekyll-feed#202 and confirmed via #6679

Basically with this patch,

Site runs as usual if the collections_dir setting is not customized.
When the collections_dir setting is customized, Jekyll looks for all collections within that directory only:
- Posts and drafts at the root will no longer be read / processed — you need to move them inside the collections_dir
- Relative path of all "documents" will be with-respect-to that directory. e.g. If the directory is named my_collections, then the relative_path for ./my_collections/_puppies/rover.md is simply _puppies/rover.md
- Ergo, relative_path for a document does not change whether the collection is at the site-root or at the root of the collection_dir

localheinz · 2018-01-08T12:33:31Z

@ashmaroli

As a note, what I tripped over initially was that I assumed that a collections directory would be special directory (similar to _data, _includes, etc), so after upgrading from 3.6.2 to 3.7.0, my initial assumption was that with a configuration of

collections_dir: _collections

and a directory layout similar to

.
├── _collections
│   ├── books
│   └── puppies
├── _data
├── _includes
├── _layouts
└── _posts

everything would be neatly arranged, and work fine.

Note:

collections directory prefixed with _ (so special directories are all in order)
directories for collections within collections directory without _ prefix

Just felt like throwing this in, as an FYI.

Crunch09

👍 Looks good, just some small things. Can we close #6679 as this supersedes that one?

Crunch09 · 2018-01-08T22:44:36Z

features/collections_dir.feature

+    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/post-at-root.html" file should not exist


I understand it doesn't exist anyway but shouldn't that be _site/2009/03/27/post-at-root.html ?

yes.. it should.. we ought to be checking for the expected outcomes.. 👍

Crunch09 · 2018-01-08T22:47:45Z

features/collections_dir.feature

+    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/gathering/2009/03/27/draft-in-gathering.html" file should not exist


A couple of lines below we check that the _site/gathering directory doesn't exist, so no need to check for a file in a non-existing directory i think.

Crunch09 · 2018-01-08T22:51:01Z

features/step_definitions.rb

+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")


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

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..

Crunch09 · 2018-01-08T22:56:29Z

lib/jekyll/reader.rb

+    # outside that directory.
+    def not_within_configured_directory?(dir)
+      collections_dir = site.config["collections_dir"]
+      !collections_dir.empty? && !dir.start_with?("/#{collections_dir}")


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

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

ashmaroli · 2018-01-09T07:33:41Z

@localheinz I understand your suggestion completely.
While I agree that the custom collections_dir with a leading underscore would allow greater aesthetics, I'm not in favor of removing the underscore from the directory for collections for reasons below:

having the leading underscore will allow greater flexibility when moving the collections en bulk into and outside the custom container (during migration and testing across older and newer versions of Jekyll)
if we decide to add support for categories in collections, similar to hows posts have it with e.g. movies/_posts/*.markdown, having the leading underscore in the path would allow writing simpler code and also allow users to mentally scan for their actual collection directory in a file tree

localheinz · 2018-01-09T07:46:46Z

@ashmaroli

I'm totally fine with _ and without, so all good. Thanks a lot for your efforts towards fixing these issues! 👍

DirtyF

Thanks for adding those tests !

Minor concerns:

Even if we document this on the website or in the default _config.yml generated by jekyll new, we should warn people that if a collection_dir is defined they should move _posts and _drafts into _collection_dir. Tests only check for a zero exit code, but it should trigger a warning in those use cases. This could be tackled in a new PR.
Out of curiosity, why do we add a date to the collection documents here?

📦 Happy to release a 3.7.1 patch once the team is onboard with this 😄

ashmaroli · 2018-01-09T08:05:11Z

why do we add a date to the collection documents here?

Its not required at all, but existing scenarios had them, so I included them here as well.. will make it easier if future developers need to test for some other date-related parameter..

parkr · 2018-01-14T16:13:09Z

features/collections_dir.feature

+    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


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.

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..

parkr · 2018-01-14T16:14:05Z

features/collections_dir.feature

+    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


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

parkr · 2018-01-14T16:15:08Z

lib/jekyll/collection.rb

-      @relative_directory ||= Pathname.new(directory).relative_path_from(
-        Pathname.new(site.source)
-      ).to_s
+      @relative_directory ||= "_#{label}"


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

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

parkr · 2018-01-14T16:16:04Z

lib/jekyll/document.rb

@@ -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


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)?

collection does not have an instance method :path

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.

Any reason...?

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

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.

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

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).

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..

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

Static files within a collection inside a custom collections_dir needs to output to the same path as when the collection is at the root of the source directory.

ghost · 2018-01-25T15:45:34Z

@jekyllbot: merge +bug

Refactor collections_dir feature for consistency

a09b646

ashmaroli requested review from parkr, pathawks and Crunch09 January 8, 2018 12:10

ashmaroli added the fix label Jan 8, 2018

DirtyF requested a review from a team January 8, 2018 12:54

Crunch09 reviewed Jan 8, 2018

View reviewed changes

make corrections based on feedback

39d0bad

ashmaroli mentioned this pull request Jan 9, 2018

Add some tests for custom collections directory #6679

Closed

ashmaroli force-pushed the fix-collections-dir branch from a3f2a9f to 39d0bad Compare January 9, 2018 07:42

DirtyF approved these changes Jan 9, 2018

View reviewed changes

ashmaroli added this to the v3.7.1 milestone Jan 11, 2018

parkr requested changes Jan 14, 2018

View reviewed changes

ashmaroli mentioned this pull request Jan 15, 2018

Release v3.7.1 #6695

Merged

7 tasks

DirtyF mentioned this pull request Jan 15, 2018

Check for collections_dir jekyll/jekyll-compose#62

Closed

props @DirtyF: handle static files in a collection

2af6834

Static files within a collection inside a custom collections_dir needs to output to the same path as when the collection is at the root of the source directory.

ashmaroli force-pushed the fix-collections-dir branch from bfe2a2c to 2af6834 Compare January 15, 2018 17:38

compute collections_path once per build session

f4daf2e

ghost approved these changes Jan 23, 2018

View reviewed changes

jekyllbot merged commit a6b4ce0 into jekyll:master Jan 25, 2018

jekyllbot added bug fix labels Jan 25, 2018

jekyllbot added a commit that referenced this pull request Jan 25, 2018

Update history to reflect merge of #6685 [ci skip]

6415458

ashmaroli deleted the fix-collections-dir branch January 29, 2018 16:51

hosnas mentioned this pull request Feb 16, 2018

supporting collections? nhoizey/jekyll-postfiles#10

Open

ashmaroli mentioned this pull request Mar 1, 2018

Include_relative liquid tag inside collection doc doesnt work #6816

Closed

5 tasks

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor collections_dir feature for consistency #6685

Refactor collections_dir feature for consistency #6685

ashmaroli commented Jan 8, 2018 •

edited

localheinz commented Jan 8, 2018 •

edited

Crunch09 left a comment

Crunch09 Jan 8, 2018

ashmaroli Jan 9, 2018

Crunch09 Jan 8, 2018

Crunch09 Jan 8, 2018

ashmaroli Jan 9, 2018

Crunch09 Jan 8, 2018

ashmaroli Jan 9, 2018

ashmaroli commented Jan 9, 2018

localheinz commented Jan 9, 2018

DirtyF left a comment

ashmaroli commented Jan 9, 2018

parkr Jan 14, 2018

ashmaroli Jan 14, 2018

parkr Jan 14, 2018

parkr Jan 14, 2018

ashmaroli Jan 14, 2018

parkr Jan 14, 2018

ashmaroli Jan 14, 2018

parkr Jan 20, 2018

ashmaroli Jan 21, 2018

DirtyF Jan 21, 2018 •

edited

ashmaroli Jan 22, 2018

parkr Jan 22, 2018

ashmaroli Jan 22, 2018

ashmaroli Jan 22, 2018

ghost commented Jan 25, 2018

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

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

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

Refactor collections_dir feature for consistency #6685

Refactor collections_dir feature for consistency #6685

Conversation

ashmaroli commented Jan 8, 2018 • edited

localheinz commented Jan 8, 2018 • edited

Crunch09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli commented Jan 9, 2018

localheinz commented Jan 9, 2018

DirtyF left a comment

Choose a reason for hiding this comment

ashmaroli commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirtyF Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 25, 2018

ashmaroli commented Jan 8, 2018 •

edited

localheinz commented Jan 8, 2018 •

edited

DirtyF Jan 21, 2018 •

edited