Added config includes to solve #514 and #703 #896

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

voidfiles commented Mar 29, 2013

This is originally from this checkin.
https://github.com/xenji/jekyll/commit/29612017f6a84eb02714693738d5031209bfd87a

I have only tried to cherry pick it on top of the current source
tree

Added config includes to solve #514 and #703
including the corresponding tests and fixtures.

You can now use a YAML list to add configuration files to be included
in the order of the list. The main configuration is included at the
final step to ensure that every value can be overridden.

The syntax supports globs (to be used with Dir[pattern]), relative
paths to the config value of source and absolute paths.

Using this sample config:

additional_configs:
  - additional_configs/_config_a.yml
  - additional_configs/_config_b.yml

or the glob version:

additional_configs:
  - additional_configs/_config_*.yml

the new deep merge order is in both cases:
Jekyll::DEFAULTS < _config_a < _config_b < _config

Using globs means to rely on the sorting that Dir[pattern] resolves.

I've added additional tests for three use cases:

  • Having one inclusion
  • Having a glob inclusion
  • Having one inclusion with override in the main config
Contributor

voidfiles commented Mar 30, 2013

I ran the tests in 1.8.7 and everything ran fine. I am not sure what to do about the failed travis build.

Contributor

maul-esel commented Mar 30, 2013

When doing a glob include, is the order in which the files are processed always the same?

Note that case sensitivity depends on your system (so File::FNM_CASEFOLD is ignored), as does the order in which the results are returned.

http://ruby-doc.org/core-2.0/Dir.html#method-c-glob

Cause it seems to me you expect config_b to override config_a:

<"/buzz"> expected but was
<"/foobar">.

Edit: also, the failure only occurs in Ruby 1.9.2, 1.8.7 is fine on travis as well.

Contributor

voidfiles commented Mar 30, 2013

Thanks, for the tip. I will look into that.

Contributor

voidfiles commented Mar 30, 2013

Thanks @maul-esel that fixed it.

Owner

parkr commented Apr 1, 2013

I'm not sure I like the idea of the Configuration class.

Contributor

voidfiles commented Apr 1, 2013

Okay, do you think it should be done just inline in jekyll.rb?

I talked a little with @imathis, about this patch and I am going to be making some changes to the order in which files get included, and I am open to more changes.

Owner

parkr commented Apr 1, 2013

With the level of manipulation being done, it makes sense to have a Configuration class. It just feels weird because the configuration has been in jekyll.rb almost since the project was started.

Cool, I should get a notification when you push/comment so I'll check those.

Also, no need to squash all your commits. It's a huge set of changes.

voidfiles added some commits Mar 29, 2013

Added config includes to solve #514 and #703
This is origonaly form this checkin.
xenji/jekyll@2961201

I have only tried to cherry pick it on top of the current source
tree
including the corresponding tests and fixtures.

You can now use a YAML list to add configuration files to be included
in the order of the list.

The syntax supports globs (to be used with `Dir[pattern]`), relative
paths to the config value of `source` and absolute paths.

Using this sample config:
```
additional_configs:
  - additional_configs/_config_a.yml
  - additional_configs/_config_b.yml
```
or the glob version:
```
additional_configs:
  - additional_configs/_config_*.yml
```
the new deep merge order is in both cases:
`Jekyll::DEFAULTS < _config < _config_a < _config_b`

I've added additional tests for three use cases:
* Having one inclusion
* Having a glob inclusion
* Having one inclusion with override in the main config

Conflicts:
	lib/jekyll/configuration.rb
	test/test_configuration.rb
Changed name of key and import order
After talking with @imathis I was convinced that the over ride beha-
vior should be something like this.

DEFAULTS < configs* < root _config.yml

Also I changed the name of the key to configs instead of
additional_configs
Owner

parkr commented Apr 2, 2013

I still feel like new config files should not be added via any other config. I'd rather have any further configs accepted via the -c/--config. Just feels weird to be defining config file dependencies in the config files.

Contributor

voidfiles commented Apr 2, 2013

My only counterpoint here is that octopress is built around the idea of
their being a _config.yml file, and only one. That can change, and it
sounds like 2.1 won't actually distribute a _config.yml. Right now though,
that's how it's setup.

I don't mind doing this through the command line, I just think that this
feature should exist in some form.

I don't know how decisions get made here, so I will defer to the community
on this one.

On Tue, Apr 2, 2013 at 3:17 PM, Parker Moore notifications@github.comwrote:

I still feel like new config files should not be added via any other
config. I'd rather have any further configs accepted via the -c/--config.
Just feels weird to be defining config file dependencies in the config
files.


Reply to this email directly or view it on GitHubhttps://github.com/mojombo/jekyll/pull/896#issuecomment-15806372
.

Contributor

mojombo commented Apr 7, 2013

I'd prefer to handle multiple configs on the command line for now. Adding it to the config file itself seems like too much complexity. If serious use cases emerge for config file includes after multiple command line includes are implemented, I'd be open to reconsidering.

@mojombo mojombo closed this Apr 7, 2013

Contributor

voidfiles commented Apr 7, 2013

Thanks for the clarification. Like I said, I am approach agnostic to this feature. It's just something I would love to have in core. I will submit another pull request that uses the command line approach.

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