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

Fix up collections #2238

Merged
merged 15 commits into from Apr 26, 2014

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Apr 16, 2014

  • Reset the collections hash on #reset. (Fixes #2234. Fixes #2233.)
  • Fix up collections config key:
#1. Allow array
collections:
  - my_collection

#2. Allow hash
collections:
  my_collection:
    hi: there

#3. Ask for `write` to write out individual output files for each document
collections:
  my_collection:
    write: true
@parkr

This comment has been minimized.

Member

parkr commented Apr 16, 2014

@benbalter I really don't like having 2 keys for mananging collections (read & write). I want them to be part of the same block:

collections:
  read:
    my_collection:
      some_metadata: "its value yo"
  write:
    - my_collection

I think it needs to be together. Looking at that vs what we have now gives me the fresh feeling like I've just brushed my teeth... but for my brain.

@parkr parkr referenced this pull request Apr 16, 2014

Closed

Add metadata to collections #2230

@mscharley

This comment has been minimized.

Contributor

mscharley commented Apr 16, 2014

@parkr Should the write section be an array?

@mscharley

This comment has been minimized.

Contributor

mscharley commented Apr 16, 2014

Here's another idea,

collections:
  readonly:
    my_collection:
       metadata: "it's value yo"
  write:
    some_other_collection:

@parkr parkr self-assigned this Apr 21, 2014

@parkr

This comment has been minimized.

Member

parkr commented Apr 22, 2014

What about

collections:
  my_collection:
    read: true
    write: true
    funky_town: "it's value yo"

?

@benbalter

This comment has been minimized.

Contributor

benbalter commented Apr 22, 2014

Read and write are "should Jekyll read in this collection" and "should Jekyll write each document in this collection out to disk individually"? If so, isn't read a prereq for write? Taking that further, isnt' read a prereq for anything (e.g., the collection existing)?

@parkr

This comment has been minimized.

Member

parkr commented Apr 22, 2014

If so, isn't read a prereq for write? Taking that further, isnt' read a prereq for anything (e.g., the collection existing)?

Yep, you'll always have to read if you want to use the collection. The real obstacle here is "is collections an Array or Hash? If it's a Hash, we need to have some value, like

collections:
  my_collection: true

or something.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Apr 22, 2014

SafeYAML correctly parses:

collections:
  test:
  something_else:
    write: true

as:

{"collections"=>{"test"=>nil, "something_else"=>{"write"=>true}}}
@parkr

This comment has been minimized.

Member

parkr commented Apr 24, 2014

Ok I am going to go with:

# 1. Allow array
collections:
  - my_collection

# 2. Allow hash
collections:
  my_collection:
    hi: there

# 3. Ask for `write` to write out individual output files for each document
collections:
  my_collection:
    write: true

@parkr parkr changed the title from WIP: Fix up collections to Fix up collections Apr 24, 2014

@parkr

This comment has been minimized.

Member

parkr commented Apr 24, 2014

@benbalter Have time to review this today? @mattr- is out of commission. If not, no biggy.

@benbalter

This comment has been minimized.

Contributor

benbalter commented Apr 24, 2014

Why'd you go with "write". That feels a lot like it's a term from our perspective (the task we perform for the user, the verb, is writing to disk), but is that what the user actually cares about?

@parkr

This comment has been minimized.

Member

parkr commented Apr 24, 2014

Why'd you go with "write". That feels a lot like it's a term from our perspective (the task we perform for the user, the verb, is writing to disk), but is that what the user actually cares about?

Maybe output? render won't work as it's too ambiguous.

@mscharley

This comment has been minimized.

Contributor

mscharley commented Apr 24, 2014

@parkr Will users possibly get confused and try:

collections:
  - my_collection
  test:
    write: true

What's wrong with using nil as the value instead of using an array?

@parkr

This comment has been minimized.

Member

parkr commented Apr 25, 2014

What's wrong with using nil as the value instead of using an array?

It feels more confusing to beginners. "I have to add a colon for this to work? What?"

@mscharley

This comment has been minimized.

Contributor

mscharley commented Apr 25, 2014

How is that any less confusing than "Oh, I have to add a hyphen in front of this, but only if I don't want to add any extra data or output them to files, then I need to use colons after it." Also, how do you intend to handle the case where only some collections have metadata/are output? You're back to square one at that point. If you'd rather not have it blank, encourage the following usage:

collections:
  test: {}
  my_collection:
    output: true

This will populate collections['test'] with an empty hash. I still feel the empty solution is better though.

parkr added a commit that referenced this pull request Apr 26, 2014

@parkr parkr merged commit 1d8fff7 into master Apr 26, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@parkr parkr deleted the fix-the-collections branch Apr 26, 2014

parkr added a commit that referenced this pull request Apr 26, 2014

parkr added a commit that referenced this pull request Apr 26, 2014

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