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

Configuration: allow users to specify a collections.posts.permalink directly #4753

Merged
merged 11 commits into from Apr 13, 2016

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Apr 4, 2016

…instead of overwriting with permalink.

If you have a config file with

permalink: date
collections:
  posts:
    output: true
    permalink: /blog/:title/

then Post permalinks will match permalink (i.e. "/:categories/:year/:month/:day/:title:output_ext") instead of collections.posts.permalink (i.e. "/blog/:title/")

I'd like to ship a 3.0.4 for this and push it out to GitHub Pages.

/cc @benbalter @jekyll/gh-pages @jekyll/core

Configuration: allow users to specify a collections.posts.permalink d…
…irectly instead of overwriting with permalink

@parkr parkr added the bug label Apr 4, 2016

@parkr parkr added this to the 3.0.4 milestone Apr 4, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Apr 4, 2016

Contributor

So this would be per-collection permalinks? (with a global default)

Contributor

benbalter commented Apr 4, 2016

So this would be per-collection permalinks? (with a global default)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2016

Member

So this would be per-collection permalinks?

@benbalter Precisely! permalink is very broad and I believe we should phase it out in favor of collection-specific permalinks (not sure what to do about pages, but that's not entirely relevant to this PR).

Since v3.0, collections have offered a permalink key as a means of setting up the URL template for its constituent documents. In 3.0.3, we set it outright to the value of permalink. This PR simply defaults it instead, allowing 3.0.4 users to be more specific in their permalink selection.

Member

parkr commented Apr 4, 2016

So this would be per-collection permalinks?

@benbalter Precisely! permalink is very broad and I believe we should phase it out in favor of collection-specific permalinks (not sure what to do about pages, but that's not entirely relevant to this PR).

Since v3.0, collections have offered a permalink key as a means of setting up the URL template for its constituent documents. In 3.0.3, we set it outright to the value of permalink. This PR simply defaults it instead, allowing 3.0.4 users to be more specific in their permalink selection.

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Apr 4, 2016

Contributor

Makes sense to me. That's how I thought it worked, TBH. 😄

Contributor

benbalter commented Apr 4, 2016

Makes sense to me. That's how I thought it worked, TBH. 😄

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2016

Member

Turns out our tests are overwriting Jekyll::Configuration::DEFAULTS, which isn't good! Specifically, sub-hashes aren't being refreshed.

Fixed using a hack and pushed for now. Would be nice to have #deep_dup one day.

Member

parkr commented Apr 4, 2016

Turns out our tests are overwriting Jekyll::Configuration::DEFAULTS, which isn't good! Specifically, sub-hashes aren't being refreshed.

Fixed using a hack and pushed for now. Would be nice to have #deep_dup one day.

parkr added some commits Apr 4, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2016

Member

/cc @jekyll/stability, don't think I broke anything but 👋 anyway

Member

parkr commented Apr 4, 2016

/cc @jekyll/stability, don't think I broke anything but 👋 anyway

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Apr 4, 2016

Member

LGTM

Member

oe commented Apr 4, 2016

LGTM

@@ -177,6 +177,7 @@ def read_config_files(files)
begin
files.each do |config_file|
next if config_file.nil? or config_file.empty?

This comment has been minimized.

@envygeeks

envygeeks Apr 4, 2016

Contributor

This is overly defensive while being non-defensive at the same time. What I mean is it's better to just do next unless config_file instead of the empty because what happens if they send true? Or a symbol, or otherwise. It might all still be toppling down.

@envygeeks

envygeeks Apr 4, 2016

Contributor

This is overly defensive while being non-defensive at the same time. What I mean is it's better to just do next unless config_file instead of the empty because what happens if they send true? Or a symbol, or otherwise. It might all still be toppling down.

This comment has been minimized.

@parkr

parkr Apr 4, 2016

Member

If you send { "config" => "" }, we try to load the file. That's not such a great thing. What's a better way which preserves some of the defensiveness?

@parkr

parkr Apr 4, 2016

Member

If you send { "config" => "" }, we try to load the file. That's not such a great thing. What's a better way which preserves some of the defensiveness?

Show outdated Hide outdated lib/jekyll.rb
@@ -96,7 +96,7 @@ def env
#
# Returns the final configuration Hash.
def configuration(override = Hash.new)
config = Configuration[Configuration::DEFAULTS]
config = Configuration[Marshal.load(Marshal.dump(Configuration::DEFAULTS))]

This comment has been minimized.

@envygeeks

envygeeks Apr 4, 2016

Contributor

What is this trying to do? Why not just freeze the hash?

@envygeeks

envygeeks Apr 4, 2016

Contributor

What is this trying to do? Why not just freeze the hash?

This comment has been minimized.

@parkr

parkr Apr 4, 2016

Member

It makes a deep duplicate of the defaults. The PR body describes the problem of modifying the defaults in some situations.

@parkr

parkr Apr 4, 2016

Member

It makes a deep duplicate of the defaults. The PR body describes the problem of modifying the defaults in some situations.

end
def test_dir(*subdirs)
File.join(File.dirname(__FILE__), *subdirs)

This comment has been minimized.

@envygeeks

envygeeks Apr 4, 2016

Contributor

File.join(__dir__, *subdirs)

@envygeeks

envygeeks Apr 4, 2016

Contributor

File.join(__dir__, *subdirs)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 4, 2016

Contributor

Recently I was told not to Marshal for security reasons that were wild and overly broad for doing the exact same thing happening here, what makes this instance different and okay? (This only applies to Jekyll's context.)

Contributor

envygeeks commented Apr 4, 2016

Recently I was told not to Marshal for security reasons that were wild and overly broad for doing the exact same thing happening here, what makes this instance different and okay? (This only applies to Jekyll's context.)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2016

Member

Recently I was told not to Marshal for security reasons that were wild and overly broad for doing the exact same thing happening here, what makes this instance different and okay?

I removed it from the runtime usage and now only using it to make a deep duplicate in the tests.

Member

parkr commented Apr 4, 2016

Recently I was told not to Marshal for security reasons that were wild and overly broad for doing the exact same thing happening here, what makes this instance different and okay?

I removed it from the runtime usage and now only using it to make a deep duplicate in the tests.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 4, 2016

Member

@envygeeks Removing the deep duplication causes this PR to fail CI. Know how we can make deep_merge_hashes duplicate properly?

Member

parkr commented Apr 4, 2016

@envygeeks Removing the deep duplication causes this PR to fail CI. Know how we can make deep_merge_hashes duplicate properly?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Apr 5, 2016

Contributor

@parkr if you wish to avoid Marshal you can do:

Hash[{ :hello => :world }.frozen.to_a].frozen? # => false
Contributor

envygeeks commented Apr 5, 2016

@parkr if you wish to avoid Marshal you can do:

Hash[{ :hello => :world }.frozen.to_a].frozen? # => false

parkr added some commits Apr 8, 2016

Add Configuration.from & use in Jekyll.configuration.
This process streamlines the creation of new configurations. Creating a new
site will choke if not all the correct options are given.
Configuration.from will ensure the overrides have all string keys and
ensures all the common issues & defaults are in place so a Site can be
created.

A common use:

    config = Configuration.from({ 'permalink' => '/:title/' }) # etc
    site = Jekyll::Site.new(config)
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 8, 2016

Member

@envygeeks This seems to work much more nicely. DEFAULTS['collections'].object_id was the same as config['collections'].object_id in add_default_collections so when we added or changed anything there, the defaults were also changed.

Member

parkr commented Apr 8, 2016

@envygeeks This seems to work much more nicely. DEFAULTS['collections'].object_id was the same as config['collections'].object_id in add_default_collections so when we added or changed anything there, the defaults were also changed.

@parkr parkr merged commit 27f1ada into 3.0-stable Apr 13, 2016

2 checks passed

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

@parkr parkr deleted the parkr/permalink-fix branch Apr 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment