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

Try to load _config.yaml if _config.yml is nonexistent. #2406

Merged
merged 1 commit into from May 20, 2014

Conversation

Projects
None yet
3 participants
@yihangho
Contributor

yihangho commented May 14, 2014

Fix #2375.

Show outdated Hide outdated lib/jekyll/configuration.rb
Jekyll.logger.warn "Configuration file:", "none"
{}
if file.end_with?('yml')
read_config_file(file.sub(/yml$/, 'yaml'))

This comment has been minimized.

@parkr

parkr May 15, 2014

Member

Shouldn't we just do a check for existence instead of doing this? I'd rather do this above, on line 126.

@parkr

parkr May 15, 2014

Member

Shouldn't we just do a check for existence instead of doing this? I'd rather do this above, on line 126.

This comment has been minimized.

@yihangho

yihangho May 15, 2014

Contributor

Thought that it would be nice if all the logic dealing with file existence and the like in one method.

Anyway, if we do the checking on line 126, I assume the default when both _config.yml and _config.yaml exist is still _config.yml, and let read_config_file takes care of the warning message?

@yihangho

yihangho May 15, 2014

Contributor

Thought that it would be nice if all the logic dealing with file existence and the like in one method.

Anyway, if we do the checking on line 126, I assume the default when both _config.yml and _config.yaml exist is still _config.yml, and let read_config_file takes care of the warning message?

This comment has been minimized.

@parkr

parkr May 15, 2014

Member

I'd say if they both exist, then yes, _config.yml should take precedence.

@parkr

parkr May 15, 2014

Member

I'd say if they both exist, then yes, _config.yml should take precedence.

Show outdated Hide outdated lib/jekyll/configuration.rb
config_files = yml_path
else
config_files = yaml_path
end

This comment has been minimized.

@parkr

parkr May 15, 2014

Member

There has to be a more functional way to do this. Perhaps:

default = %w[yml yaml].find do |ext|
  File.exist? Jekyll.sanitized_path(source(override), "_config.#{ext}")
end
if default.nil?
  @default_config_file = true
end
@parkr

parkr May 15, 2014

Member

There has to be a more functional way to do this. Perhaps:

default = %w[yml yaml].find do |ext|
  File.exist? Jekyll.sanitized_path(source(override), "_config.#{ext}")
end
if default.nil?
  @default_config_file = true
end

This comment has been minimized.

@yihangho

yihangho May 16, 2014

Contributor

Cool! That's a good idea.

@yihangho

yihangho May 16, 2014

Contributor

Cool! That's a good idea.

This comment has been minimized.

@yihangho

yihangho May 16, 2014

Contributor

But shouldn't @default_config_file be set to true even if both .yml and .yaml do not exist so that #read_config_file can set off the proper warning?

@yihangho

yihangho May 16, 2014

Contributor

But shouldn't @default_config_file be set to true even if both .yml and .yaml do not exist so that #read_config_file can set off the proper warning?

This comment has been minimized.

@parkr

parkr May 16, 2014

Member

Yep, you're right. Just remove the if around that var.

@parkr

parkr May 16, 2014

Member

Yep, you're right. Just remove the if around that var.

This comment has been minimized.

@yihangho

yihangho May 18, 2014

Contributor

How about now?

@yihangho

yihangho May 18, 2014

Contributor

How about now?

@@ -123,7 +123,10 @@ def config_files(override)
# Get configuration from <source>/_config.yml or <source>/<config_file>
config_files = override.delete('config')
if config_files.to_s.empty?
config_files = File.join(source(override), "_config.yml")
default = %w[yml yaml].find(Proc.new { 'yml' }) do |ext|

This comment has been minimized.

@parkr

parkr May 18, 2014

Member

Why do you need this Proc? That shouldn't be needed at all.

@parkr

parkr May 18, 2014

Member

Why do you need this Proc? That shouldn't be needed at all.

This comment has been minimized.

@yihangho

yihangho May 18, 2014

Contributor

The Proc is there to give the default value, that is, yml when both yml and yaml do not exist. This is the behavior as before.

@yihangho

yihangho May 18, 2014

Contributor

The Proc is there to give the default value, that is, yml when both yml and yaml do not exist. This is the behavior as before.

This comment has been minimized.

@parkr

parkr May 18, 2014

Member

Ah, ok, great. Thanks!

@parkr

parkr May 18, 2014

Member

Ah, ok, great. Thanks!

Show outdated Hide outdated test/test_configuration.rb
end
should "return .yml if both .yml and .yaml exist" do
mock(File).exists?(File.join(source_dir, "_config.yml")) { true }
assert_equal [File.join(source_dir, "_config.yml")], @config.config_files(@no_override)

This comment has been minimized.

@parkr

parkr May 18, 2014

Member

Do source_dir("_config.yml")here instead of File.join ....

@parkr

parkr May 18, 2014

Member

Do source_dir("_config.yml")here instead of File.join ....

This comment has been minimized.

@yihangho

yihangho May 18, 2014

Contributor

Cool, never knew there is this helper method. Should I also change the File.join on line 42 to use this method as well?

@yihangho

yihangho May 18, 2014

Contributor

Cool, never knew there is this helper method. Should I also change the File.join on line 42 to use this method as well?

This comment has been minimized.

@parkr

parkr May 18, 2014

Member

If you'd like to!

@parkr

parkr May 18, 2014

Member

If you'd like to!

This comment has been minimized.

@yihangho

yihangho May 19, 2014

Contributor

Done!

@yihangho

yihangho May 19, 2014

Contributor

Done!

Show outdated Hide outdated test/test_configuration.rb
should "return .yaml if it exists but .yml does not" do
mock(File).exists?(Jekyll.sanitized_path(source_dir, "_config.yml")) { false }
mock(File).exists?(Jekyll.sanitized_path(source_dir, "_config.yaml")) { true }
assert_equal [Jekyll.sanitized_path(source_dir, "_config.yaml")], @config.config_files(@no_override)

This comment has been minimized.

@parkr

parkr May 18, 2014

Member

Same as below – Jekyll.sanitized_path isn't necessary for tests. Use source_dir("_config.yaml")

@parkr

parkr May 18, 2014

Member

Same as below – Jekyll.sanitized_path isn't necessary for tests. Use source_dir("_config.yaml")

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 20, 2014

Member

Sweet, LGTM.

Member

parkr commented May 20, 2014

Sweet, LGTM.

parkr added a commit that referenced this pull request May 20, 2014

@parkr parkr merged commit 54fba9a into jekyll:master May 20, 2014

1 check passed

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

parkr added a commit that referenced this pull request May 20, 2014

@yihangho yihangho deleted the yihangho:config-yaml branch May 20, 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.