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

Call me collections #2199

Merged
merged 30 commits into from
Apr 15, 2014
Merged

Call me collections #2199

merged 30 commits into from
Apr 15, 2014

Conversation

parkr
Copy link
Member

@parkr parkr commented Apr 2, 2014

  • Use Collections for data feature
  • Allow us to read in collections
  • Render collections
  • TomDoc the sh*t outta this code
  • Write docs
  • Expose to Liquid via site.*

/cc @gjtorikian

Fixes #1941.

And I have a configuration file with:
| key | value |
| collections | ['methods'] |
| render | \n methods: /methods/:subdir/:title:extname |
Copy link
Member Author

Choose a reason for hiding this comment

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

This compiles to:

collections: ["methods"]
render:
  methods: /methods/:subdir/:title:extname

Specifying the output URL only in one key rather than trying to clump things and over-complicate collections key.

Thoughts on this, @gjtorikian & @benbalter?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense. The : notation leaves a bit to be desired. I think what bothers me is seeing it in such close succession: :title:extname. Is this the only possibility, or can another syntax be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gjtorikian Other option is to do string interpolation:

render:
  methods: "/methods/#{subdir}/#{title}#{extname}"

Would just use ERB to inject the variable values and boom done.

Downside: inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really like this. Two questions:

  1. What's subdir?
  2. Don't :'s in values bork YAML sans quotes?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. :subdir is the subdirectory that contains the file, i.e. the folders b/w the collection folder and the file. E.g. jekyll is the subdir for _methods/jekyll/configuration.md.

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't that be :path?

:path would include the filename.

And when would I want any configuration other than :path/:file:extension?

When I want to prefix all the files with something.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I want to prefix all the files with something.

Do you have an example that would be >= 20% of the use cases? Feels like an edge case to me that adds a lot of complexity and could be resolved by simply using a different naming pattern for the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

So in the case of _methods/site/generate.md, what would you expect the output path to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

So options one, it's always /:collection_name/:path, unless you specify a permlink. Option two, you specify a permalink structure of /:collection/:path and get /methods/site/generator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah those are the options. The question is whether we should allow the user to specify. Maybe that's a 2.1.0 optimization. PUNT. 🏈

@parkr parkr added this to the 2.0 milestone Apr 2, 2014
@parkr parkr added the Feature label Apr 2, 2014
{
"content" => output,
"page" => document.to_liquid,
"layout" => layout.data
Copy link
Member Author

Choose a reason for hiding this comment

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

@penibelst What if I add this for layouts? Re: #2070

Copy link
Member

Choose a reason for hiding this comment

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

@parkr Sorry, I can’t judge if it solve the issue.

@benbalter
Copy link
Contributor

The vision is to eventually 🐶food this for Pages and Posts, right? 😄

@benbalter
Copy link
Contributor

May want to make notes in the docs that this is an experimental feature and that the API may likely change until the feature stabilizes. Can help manage expectations and give us a bit more flexibility around SemVer.

@parkr
Copy link
Member Author

parkr commented Apr 3, 2014

@benbalter Thank you times a million for reading through this and commenting. So freakin helpful.

The vision is to eventually 🐶food this for Pages and Posts, right?

ja, but I want to get an initial version out there. There are a larrrrge # of further questions in order to turn pages and posts into collections (like using the Page and Post classes) so I'd rather do that in a different PR.

{
"label" => label,
"docs" => docs
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@benbalter Do you think we should just send back the docs array instead of the label, too? It then behaves like site.posts:

{% for datafile in site.collections.data %}
  {{ datafile.foo }} {{ datafile.bar }}
{% endfor %}

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not make things harder down the line, or build out a new design pattern.

I'd expose collections as a first class citizen like we do posts or pages, so a documents collection should live as site.documents and I should iterate through it just like I would site.posts or site.pages.

Copy link
Member Author

Choose a reason for hiding this comment

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

"a first class citizen"

so a documents collection should live as site.documents

I'm really weary of this in terms of configuration collisions or future features. What's so bad about namespacing them in site.collections?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's so bad about namespacing them in site.collections?

Because pages, posts, and data would be first-class citizens in Jekyll's eyes, but my collections would be a second class citizen (Not to mention, would make dogfooding 999x harder).

User is in control of both site config vars and collection names. Assuming that a collection name is plural, the potential collisions you're looking at are plugins, layouts, gems, which I'd argue, collections could just overwrite, as those three in particular are rarely access in userspace.

Copy link
Member Author

Choose a reason for hiding this comment

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

User is in control of both site config vars and collection names

Alright... this puts my mind at ease. My greatest worry about this is obviousness – will the user intuit access to these collections? If not, how can we get them as close as possible to just knowing that a given collection can be accessed in this way with this variable reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Answering your question, and another advantage is education is easier. "Collections are just like posts or pages, but you choose what they're called". Behavior is identical to existing behavior, no new site.collections.foo design/interaction pattern to learn.

parkr added a commit that referenced this pull request Apr 15, 2014
@parkr parkr merged commit cb4a7a5 into master Apr 15, 2014
@parkr parkr deleted the collection-plate branch April 15, 2014 03:14
parkr added a commit that referenced this pull request Apr 15, 2014
@benbalter
Copy link
Contributor

YESSSSSSSSSSS

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
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.

Arbitrary collection support
8 participants