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

Implement basic support for collections #155

Closed
wants to merge 1 commit into from

Conversation

Tasssadar
Copy link

This does two things:

  1. Optionally, merges tags from all collections into the global tags pool
  2. Adds post.collections attribute to the archive layout, so you can split the page by collection.

@ashmaroli
Copy link
Member

@Tasssadar Thank you for submitting this pull request.
However, I'm not so confident about the implementation here. I understand that you're probably new to Jekyll (and Ruby even). So, let me jot down a couple of stuff:

  • The collections option has meaning only when sub-option merge_tags is true ??
  • Posts are collections. So when you're expanding the functionality to user-defined collections, using the term documents would be better. Posts are a special collection with label "posts". Drafts are a special subset of the posts collection.
  • The implementations need refactoring:
    • First of all, please run script/fmt -a to have RuboCop correct simple linting offenses. Then please manually fix the remaining ones.
    • Prefer using Hash#dig over Hash#fetch along with the safe-navigation operator.
    • Prefer using Hash#each_value instead of a block with ignored first parameter.
  • Why are you asserting "tag/collection/index.html" when you test document also has a non-generic tag named "Test Tag"?

@Tasssadar
Copy link
Author

Hello,
thanks for your response!
Let me start with the purpose of this - we just needed to generate tags for custom collections, and I'm just trying to see if this is something you'd like in upstream or if you'd prefer some more complete support for custom collections (which I don't plan to develop).

* The `collections` option has meaning only when sub-option `merge_tags` is `true` ??

Yes. That's probably not so great, but merge_custom_collection_tags seemed kinda weird to me. Anyway, I assumed there are gonna be some changes to be made after you review this PR.

* Posts are collections. So when you're expanding the functionality to user-defined collections, using  the term _`documents`_ would be better. _Posts are a special collection with label `"posts"`. Drafts are a special subset of the posts collection._

That may be so in the code, but it's just Collections in the documentation, not even Custom collections - just Collections: https://jekyllrb.com/docs/collections/
Wouldn't we be inventing new terms if we'd call it "documents"?

I see now that the implementation is wrong and in wouldn't work correctly when a site uses both the hard-coded post collection and also other custom collections. What do you think should be done?

How about I change the code to only gather tags from the @site.collections, and never @site.tags? I'd also add new config option which would specify which collections to use, an it would be [ posts ] by default.

* The implementations need refactoring:
  
  * First of all, please run `script/fmt -a` to have RuboCop correct simple linting offenses. Then please manually fix the remaining ones.
  * Prefer using `Hash#dig` over `Hash#fetch` along with the safe-navigation operator.
  * Prefer using `Hash#each_value` instead of a block with ignored first parameter.

Thanks, will change that, you correctly deduced that Ruby is not my language of choice :)

* Why are you asserting `"tag/collection/index.html"` when you test document also has a non-generic tag named `"Test Tag"`?

The collections tag is also unique to the document, but you are right that it is useless to have that many tags in the test document, will change it as well.

@ashmaroli
Copy link
Member

Thank you for getting back to this. I'd like to provide clarification for some of your queries:

Wouldn't we be inventing new terms if we'd call it "documents"?

No we won't. Think of it this way:
Collections are actually directories of files that contain front matter and optionally, files that do not contain front matter. Those that contain front matter are instances of Jekyll::Document so they are referred to as documents. Those that do not contain front matter are instances of Jekyll::StaticFile so they're referred to as static files. The hard-coded collection posts from legacy times were added to this at a later date but got to keep their quirks for backwards-compatibility.

So in a Jekyll site, documents is an array consisting of:

  • Entities from the _posts directory.
  • Entities from the _drafts directory. (which are internally a subset of the posts collections!!)
  • Entities with front matter from a user-defined collection.

only gather tags from the @site.collections

That could work.. But ideally it should be handled upstream at Jekyll.
(Yeah, you're not interested in doing that...)


you are right that it is useless to have that many tags in the test document

I meant test using the compound tag "Test Tag" instead of the generic "collections". Sorry for being vague.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants