Skip to content

Loading…

Fixes 730 (picking up changes in _config.yml when run with --auto) #779

Closed
wants to merge 1 commit into from

6 participants

@why-el

This commit enables jekyll to pick up changes in the _config.yml file when run with --auto (--watch in the current master).

It does so by detecting if the set of changed files includes the _config.yml file, and if so, it updates the current site options with the hash from the config file and then processes it.

I tested this by adding a destination option to the _config.yml and it worked. The way this is currently done might be improved, as I am using the same code to both initialize and update site options. Perhaps we can selectively update options based on whats passed from the config file but I only had a couple of hours today.

This commit LACKS tests. I opened this pull request to get early feedback and get a sense of whether this is the correct approach to the issue. It obviously needs more work. If green lighted, I will add tests and update the docs.

@why-el why-el Fixes 730
This commit enables jekyll to pick up changes in the _config.yml
file when run with --auto (--watch in the current master).

It does so by detecting if the set of changed files includes the
_config.yml file, and if so, it updates the current site options
with the hash from the config file and then processes it.
fe64982
@tombell tombell commented on the diff
lib/jekyll/commands/build.rb
@@ -56,7 +58,13 @@ def self.watch(site, options)
dw.add_observer do |*args|
t = Time.now.strftime("%Y-%m-%d %H:%M:%S")
+ files_changed = args.collect {|event| event.path }
@tombell
tombell added a note

I would rename collect to map.

@why-el
why-el added a note

Oh I thought they were equivalent? Collect just makes more sense to me semantically.

@tombell
tombell added a note

Yep they're the same, map is more idiomatic to ruby.

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

Any feedback on this?

@mattr-
Jekyll member

Like @tombell, i'd prefer to see map because it's more idiomatic ruby, but even without that. it's fine.

:+1:

@why-el

Awesome. I will change that to map in the next commit.

Maybe @parkr will have few things to add.

@parkr parkr commented on the diff
lib/jekyll/commands/build.rb
@@ -56,7 +58,13 @@ def self.watch(site, options)
dw.add_observer do |*args|
t = Time.now.strftime("%Y-%m-%d %H:%M:%S")
+ files_changed = args.collect {|event| event.path }
+ if files_changed.include? (config_file)
@parkr Jekyll member
parkr added a note

Only include the space between the end of a method call and its arguments if the arguments do not have parentheses. Otherwise, no space. (please remove either the space or the parentheses)

Could you also remove the double space after the =? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@parkr parkr commented on the diff
lib/jekyll/commands/build.rb
((9 lines not shown))
puts "[#{t}] regeneration: #{args.size} files changed"
+
@parkr Jekyll member
parkr added a note

There are 6 spaces here. Please remove :-)

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

This looks pretty good to me. Going to think on the strategy a bit before I merge it - edge cases :-)

How have your tests of this been?

@why-el

Thanks all. I am writing tests/fixing formatting and documentation tomorrow. I am thinking of testing with the destination option. Any ideas or suggestions?

@parkr
Jekyll member

Test with a one option change, and with many option changes. It's more about just making sure that the methods you've rewritten work properly so we don't mess them up in the future. :-)

@why-el

Hey are we testing the reading of the _config.yml file anywhere? I can't find a test case where thats being merged with the defaults. I can resort to just calling the update_configuration method that I added to site.rb, but I think it would make sense to have an intergration test that covers reading the _config.yml a second time after the first site generation.

@why-el

Any feedback guys? Proper config file reading should be tested don't you think?

@parkr
Jekyll member

test_configuration.rb tests reading in the config. It should probably be extended though.

Why not just use the exact same method of reading in the config file when it changes? This seems weird to have 2 different ways of reading in the file and setting the options. Also, ensure that my command line args always supersede any configs in _config.yml.

@why-el

Sorry I got sidetracked with some stuff. I will resume this in the next couple of days.

@parkr
Jekyll member

Let me know when you've finished these up, @why-el.

@parkr
Jekyll member

And please rebase on current master - this branch and master have diverged a bit.

@why-el

Will do.

@parkr
Jekyll member

Any update on this? :)

@why-el

Yep. I am rebasing and you did a lot of things since the last time I checked. ;)

@why-el

Hey guys,

I think I am going to abandon this one and start the pull from scratch if thats ok with everyone. Too many changes to deal with since the last time I reviewed the code and I think I might introduce more headaches than a working feature.

@why-el why-el closed this
@parkr
Jekyll member

That sounds awesome.

@padi

@why-el wondering where that new pull request is.
#730 is still open too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 26, 2013
  1. @why-el

    Fixes 730

    why-el committed
    This commit enables jekyll to pick up changes in the _config.yml
    file when run with --auto (--watch in the current master).
    
    It does so by detecting if the set of changed files includes the
    _config.yml file, and if so, it updates the current site options
    with the hash from the config file and then processes it.
Showing with 64 additions and 5 deletions.
  1. +37 −3 lib/jekyll.rb
  2. +8 −0 lib/jekyll/commands/build.rb
  3. +19 −2 lib/jekyll/site.rb
View
40 lib/jekyll.rb
@@ -129,6 +129,42 @@ def self.configuration(override)
# Get configuration from <source>/_config.yml
config_file = File.join(source, '_config.yml')
+ config = self.read_config_file(config_file)
+
+
+ # Merge DEFAULTS < _config.yml < override
+ Jekyll::DEFAULTS.deep_merge(config).deep_merge(override)
+ end
+
+ # Public: Regenerate a Jekyll configuration Hash by merging
+ # the passed options with anything suppied in _config.yml, in this order.
+ #
+ # options - A Hash of config directives that was read when the site was
+ # initialized.
+ #
+ # Returns the updated configuration Hash.
+ def self.merge_with_config_file(options)
+ # Convert any symbol keys to strings and remove the old key/values
+ override = options.reduce({}) { |hsh,(k,v)| hsh.merge(k.to_s => v) }
+
+ # _config.yml may override default source location, but until
+ # then, we need to know where to look for _config.yml
+ source = options['source'] || Jekyll::DEFAULTS['source']
+
+ # Get configuration from <source>/_config.yml
+ config_file = File.join(source, '_config.yml')
+ config = self.read_config_file(config_file)
+
+ options.deep_merge(config)
+ end
+
+ # Private: Read options from the _config.yml file.
+ #
+ # config_file - the _config.yml.
+ #
+ # Returns the configuration Hash from _config.yml or an empty hash if errors
+ # are encountered.
+ def self.read_config_file(config_file)
begin
config = YAML.load_file(config_file)
raise "Invalid configuration - #{config_file}" if !config.is_a?(Hash)
@@ -139,8 +175,6 @@ def self.configuration(override)
$stderr.puts "\t" + err.to_s
config = {}
end
-
- # Merge DEFAULTS < _config.yml < override
- Jekyll::DEFAULTS.deep_merge(config).deep_merge(override)
+ config
end
end
View
8 lib/jekyll/commands/build.rb
@@ -47,6 +47,8 @@ def self.watch(site, options)
source = options['source']
destination = options['destination']
+ # Needed to check if changed when regenrating.
+ config_file = File.join(source, '_config.yml')
puts "Auto-Regenerating enabled: #{source} -> #{destination}"
@@ -56,7 +58,13 @@ def self.watch(site, options)
dw.add_observer do |*args|
t = Time.now.strftime("%Y-%m-%d %H:%M:%S")
+ files_changed = args.collect {|event| event.path }
@tombell
tombell added a note

I would rename collect to map.

@why-el
why-el added a note

Oh I thought they were equivalent? Collect just makes more sense to me semantically.

@tombell
tombell added a note

Yep they're the same, map is more idiomatic to ruby.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ if files_changed.include? (config_file)
@parkr Jekyll member
parkr added a note

Only include the space between the end of a method call and its arguments if the arguments do not have parentheses. Otherwise, no space. (please remove either the space or the parentheses)

Could you also remove the double space after the =? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ updated_options = Jekyll.merge_with_config_file(options)
+ site.update_configuration(updated_options)
+ end
puts "[#{t}] regeneration: #{args.size} files changed"
+
@parkr Jekyll member
parkr added a note

There are 6 spaces here. Please remove :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
site.process
end
View
21 lib/jekyll/site.rb
@@ -14,6 +14,17 @@ class Site
#
# config - A Hash containing site configuration details.
def initialize(config)
+ self.read_config_data(config)
+ self.reset
+ self.setup
+ end
+
+ # Private: Read site configuration.
+ #
+ # config - A Hash containing site configuration details.
+ #
+ # Returns nothing.
+ def read_config_data(config)
self.config = config.clone
self.safe = config['safe']
@@ -28,9 +39,15 @@ def initialize(config)
self.future = config['future']
self.limit_posts = config['limit_posts'] || nil
self.keep_files = config['keep_files'] || []
+ end
- self.reset
- self.setup
+ # Public: Update site configuration.
+ #
+ # updated_options - A Hash containing updated site configuration.
+ #
+ # Returns nothing.
+ def update_configuration(updated_options)
+ self.read_config_data(updated_options)
end
# Public: Read, process, and write this Site to output.
Something went wrong with that request. Please try again.