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

Proof of concept: Collections support #88

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Proof of concept: Collections support #88

wants to merge 3 commits into from

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Jan 20, 2017

What is this?

As it stands, this plugin only supports posts, i.e. documents in the default posts collection. Now that collections are mature and very useful, this plugin ought to support this Jekyll feature fully. This is my attempt at achieving this. Right now, it works, but it needs input and plenty of expert review.

How do I propose enabling collection archives?

To ensure backwards compatibility, documents in the default site.posts collection will continue to appear within enabled archives, but to add additional collections, you’ll need to add an archive: true value to each collection’s respective config, e.g.:

collections:
  albums:
    output: true
    archive: true

Is this the right approach? I also considered enabling archives via front matter defaults (and that way you could also exclude individual pages from being archived) but this felt like a separate feature.

Code review

Arguably, some of the code I’ve added here should be provided by Jekyll’s core. For example, Jekyll’s post_attr_hash utility only parses posts, but there doesn’t seem to be an equivalent function to parse documents across all collections; I’ve added a doc_attr_hash utility to perform that function here. Or have I missed something? It should be noted that the code within this function feels fragile even to my inexperienced eyes, so I welcome improvements, optimisations and refinement.

Next steps

  • Get consensus on approach to configuration
  • Confirm whether code should be forked to continue support for Jekyll 2.0
  • Review code and optimise
  • Add tests
  • Update documentation

I look forward to receiving feedback from the core plugin team, reviewers and others. /cc @alfredxing, @parkr, @pathawks

@paulrobertlloyd
Copy link
Contributor Author

Going by the initial CI tests, looks like more code forking might be needed to provide compatibility with Jekyll 2.0. 🤔

@alfredxing
Copy link
Member

alfredxing commented Jan 24, 2017

Thanks for the PR! Just some of my thoughts here:

  • IMO all configuration for the plugin should be contained in the jekyll-archives hash; I don't think we should pollute the default Jekyll configuration spaces.
  • Documents don't have things like tags, categories, or date "by default". Tags and categories make sense, not sure if date does?
  • On attr_hash: if you think this would be useful to Jekyll in general, we can maybe think about including it in core? If so, you can open a PR there to add it. Ideally it can be a very general utility, taking in the attributes as well as a list of collections to look in. Then post_attr_hash can be refactored to just use that.
  • Jekyll 2 compatibility: this is kind of a pain. At the very least, you can probably refactor out the common if (Jekyll::VERSION >= '3.0.0') attr_hash(...) parts into a separate function. If compatibility becomes too much work we may have to re-think some things WRT that

@paulrobertlloyd
Copy link
Contributor Author

Thanks for the feedback @alfredxing!

IMO all configuration for the plugin should be contained in the jekyll-archives hash; I don't think we should pollute the default Jekyll configuration spaces.

Makes sense. How would you imagine configuring collections? Perhaps:

jekyll-archives:
  collections: [albums, books, films]

How might we deal with the posts collection in this case? Enabled by default, but if adding a collectionsarray, posts needs to be included?

Documents don't have things like tags, categories, or date "by default". Tags and categories make sense, not sure if date does?

Documents may not have these attributes by default, but they can – so this shouldn’t matter, no? (Most of my collection documents have dates, FWIW).

On attr_hash: if you think this would be useful to Jekyll in general, we can maybe think about including it in core?

I had a feeling you might say this! Will look at the core and see if anything has been proposed already. My general sense is that the core still treats posts as a special case, when really they should be treated as a default; i.e. site.tags only looks at posts, when it should probably look across all documents. Anyway, I’ll move this particular discussion to core.

you can probably refactor out the common if (Jekyll::VERSION >= '3.0.0') attr_hash(...) parts into a separate function.

You’re overestimate my programming abilities!! But yes, I think this might need abstracting out. I’m curious as to the general approach regarding compatibility with plugins, though?

@paulrobertlloyd
Copy link
Contributor Author

@alfredxing Okay, I’ve had a stab at adding a global utility function to core (doc_attr_hash), and submitted a PR. I followed your idea of refactoring post_attr_hash to use this new function, too. I think it works well… 🤞

@DirtyF DirtyF requested a review from a team October 22, 2017 15:10
@spencertweedy
Copy link

Has there been any development on this pr?

@DirtyF DirtyF added the stale label Nov 23, 2020
@zinzy
Copy link

zinzy commented Jul 8, 2023

I know @spencertweedy already asked this question, but I wanted to reach out and ask if we might see this great initiative come to fruition. I'd be happy to make a donation if that helps!

@jekyllbot jekyllbot removed the stale label Jul 8, 2023
@positron96
Copy link

Hi all. I realized that the project has not been updated for a year, and the PR is from 2017, so I took the liberty to fork it and rebase the PR to 2.2.1 version. It seems to work for me.

To use it, I put this into Gemfile:

group :jekyll_plugins do
  gem "jekyll-archives", git: "https://github.com/positron96/jekyll-archives.git", ref: "ed7e67a"
end

@CookiePLMonster
Copy link

I'll comment only to trigger some movement on this PR and show that there is someone who cares - thank you for the fork!

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.

8 participants