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

Allow additional front matter for Post #41

Merged
merged 7 commits into from Oct 9, 2018

Conversation

toshimaru
Copy link
Contributor

@toshimaru toshimaru commented May 14, 2016

Currently, jekyll-compose post command generates only layout and title as front matter, but I'd like to add more front matters. For example:

---
layout: post
title: Blogging Like a Hacker
published: true
description: 
category: 

---

This PR adds feature that allows addtional front matter loaded from Jekyll's _config.yml for jekyll post command.

_config.yml sample is like:

# _config.yml

# jekyll-compose
jekyll_compose:
  post_default_front_matter:
    published: true
    foo: bar
    category: 

@mofhu
Copy link

mofhu commented Jul 16, 2016

Thanks a lot @toshimaru . In fact, I'm writing a short blog introducing jekyll-compose and was wondering if this feature may be added 😄

@mvdmakesthings
Copy link

I'd LOVE to see this feature merged in.

mofhu added a commit to mofhu/jekyll-compose that referenced this pull request Mar 5, 2017
able to add draft and post front matter in _config.yml in Jekyll
directory.
adopted from toshimaru's PR
(jekyll#41)
@hassek
Copy link

hassek commented Apr 23, 2017

@codethebeard Totally, I installed this package and immediately though of this feature!

@mpchadwick
Copy link

+1 to this feature. This project is not particularly useful without it for me.

@DirtyF
Copy link
Member

DirtyF commented Oct 13, 2017

@pathawks this seems complementary to #43

@pathawks
Copy link
Member

There was some online interface I saw once that allowed default YAML for posts to be stored in a way similar to this. Does that ring a bell for anybody?

If somebody out there is already doing this, we should make sure that the way we do it is complementary to that.

@courajs courajs removed their request for review October 14, 2017 13:39
@DirtyF
Copy link
Member

DirtyF commented Nov 14, 2017

@pathawks rather than adding extra options, I think jeyll-compose should use defaults defined in the config

@pathawks
Copy link
Member

I’m not sure about this.
We have added post defaults to aid this, but defaults will be overridden if each post contains a key with just an empty string.

Seems best to leave it to the authors to add only what is needed. I would argue that maybe we should stop adding layout

@toshimaru
Copy link
Contributor Author

Almost all of my posts have different description which should be configured in my jekyll site, so additional configuration will be worth it in case I forget writing description in the post.

jeyll-compose should use defaults defined in the config

Might be a good idea, but I'd like to configure post default setting separately because it has a value for the layout, not for the post.

@hmistry
Copy link

hmistry commented Nov 29, 2017

LGTM please!!! I hope this is also added to the draft command.

I'm really surprised that this capability was left out because, as others have pointed out, it's expected or very, very high on the want list. It's the main reason why I bothered to search for this plugin.

I agree with @toshimaru original approach - have a full list of common defaults, prefilled where applicable with sensible values, and allow user to configure in the _config.yml file. To me, this is equivalent to rails generate scaffold posts where it generates a complete set of templates and I modify/delete things accordingly. If rails g scaffold only created an index action and views, then it would be kind of useless by not saving the dev time from having to lookup/copy files - deleting is faster than looking up. Same goes for the draft and post command for jekyll.

@DirtyF DirtyF requested a review from a team January 14, 2018 20:20
@@ -53,6 +53,11 @@ def file_name
def _date_stamp
@params.date.strftime '%Y-%m-%d'
end

def content
post_front_matter = Jekyll.configuration["post_front_matter"]
Copy link
Member

Choose a reason for hiding this comment

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

Let's use a different configuration key here. I'd love to keep things organized under compose. What about:

compose:
  post_default_front_matter:
    my: keys
    go: here

Top-level keys often conflict with user-specific values and it's nice to keep a plugin's keys all under one namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jekyll_compose is used as a top-level namespace, so I use the same naming.

@toshimaru
Copy link
Contributor Author

@DirtyF fixed conflict. Could you review? cc/ @pathawks @parkr

@DirtyF DirtyF requested a review from a team October 7, 2018 16:58
@DirtyF DirtyF removed the request for review from pathawks October 7, 2018 16:59
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

LGTM, as jekyll_compose is already used as configuration key by auto_open.

@DirtyF DirtyF added this to the 0.9.0 milestone Oct 7, 2018
@DirtyF DirtyF added the feature label Oct 7, 2018
Copy link
Member

@ashmaroli ashmaroli left a comment

Choose a reason for hiding this comment

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

@toshimaru This would benefit from some minor changes..

private

def jekyll_compose_config
@jekyll_compose_config ||= Jekyll.configuration["jekyll_compose"]
Copy link
Member

Choose a reason for hiding this comment

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

Let's default the config to a hash so that we can avoid a nil check in method #content above:

# Returns a Hash
def compose_config
  @compose_config ||= Jekyll.configuration["jekyll_compose"] || {}
end

Copy link
Member

Choose a reason for hiding this comment

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

This has been adressed in #76

@@ -64,8 +64,18 @@ def _time_stamp
end

def content(custom_front_matter = {})
if jekyll_compose_config && jekyll_compose_config["post_default_front_matter"]
custom_front_matter.merge!(jekyll_compose_config["post_default_front_matter"])
Copy link
Member

Choose a reason for hiding this comment

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

Let's merge! only a valid config:

def content(custom_front_matter = {})
  default_front_matter = compose_config["post_default_front_matter"]
  custom_front_matter.merge!(default_front_matter) if default_front_matter.is_a?(Hash)

  super({ "date" => _time_stamp }.merge(custom_front_matter))
end

@toshimaru
Copy link
Contributor Author

@ashmaroli thanks for your review, fixed!!

@ashmaroli
Copy link
Member

@toshimaru Thank you for addressing my concerns.
The maintainers have decided to avoid using Jekyll.configuration to get the config data and instead use the configuration_from_options method from the superclass.

You may refer #76 for more details regarding the proposed change

@toshimaru
Copy link
Contributor Author

@ashmaroli So, should I wait until #76 get merged?

@DirtyF
Copy link
Member

DirtyF commented Oct 9, 2018

@toshimaru We'll deal with jekyll.configuration in #76, thanks for this useful feature.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 14e6e54 into jekyll:master Oct 9, 2018
jekyllbot added a commit that referenced this pull request Oct 9, 2018
@toshimaru toshimaru deleted the feature/post-template branch October 10, 2018 06:46
@DirtyF
Copy link
Member

DirtyF commented Oct 18, 2018

Note: this does not currently work for drafts.

@jekyll jekyll locked and limited conversation to collaborators Oct 18, 2019
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.

None yet