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

Stringify configuration overrides before first use #5060

Merged
merged 1 commit into from Jul 16, 2016

Conversation

Projects
None yet
4 participants
@stomar
Contributor

stomar commented Jul 6, 2016

This fixes a regression with configuration override options where the keys are not stringified before the first use (namely config and skip_initial_build).
The regression was introduced with version 3.1.4, commit d01f794.

See the example below (with 3.1.5, since requiring of 3.1.4 fails for me):

$ cat _my_config.yml
exclude:
  - file

$ cat test-3.1.3.rb
gem "jekyll", "3.1.3"
require "jekyll"

p Jekyll.configuration(:config  => "_my_config.yml")["exclude"]
p Jekyll.configuration("config" => "_my_config.yml")["exclude"]

$ cat test-3.1.5.rb
gem "jekyll", "3.1.5"
require "jekyll"

p Jekyll.configuration(:config  => "_my_config.yml")["exclude"]
p Jekyll.configuration("config" => "_my_config.yml")["exclude"]

Running the example with 3.1.5 produces:

Configuration file: none
[]
Configuration file: _my_config.yml
["file"]

while for 3.1.3 all is fine:

Configuration file: _my_config.yml
["file"]
Configuration file: _my_config.yml
["file"]
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 7, 2016

Member

Cool. Would you please add a test? Thanks!

Member

parkr commented Jul 7, 2016

Cool. Would you please add a test? Thanks!

@parkr parkr added this to the 3.2 milestone Jul 7, 2016

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 7, 2016

Contributor

I'm :shipit: after @parkr's requests are met.

Contributor

envygeeks commented Jul 7, 2016

I'm :shipit: after @parkr's requests are met.

@jekyllbot jekyllbot removed the needs-work label Jul 7, 2016

@@ -335,6 +335,18 @@ class TestConfiguration < JekyllUnitTest
Jekyll.configuration(test_config.merge({ "config" => @paths[:other] }))
end
should "load different config if specified with symbol key" do
allow(SafeYAML).to receive(:load_file).with(@paths[:default]).and_return({})

This comment has been minimized.

@stomar

stomar Jul 7, 2016

Contributor

(this line is not essential, but in the case when the config option is not recognized properly, this produces a regular test failure, instead of an exception due to a wrongly set up stub)

@stomar

stomar Jul 7, 2016

Contributor

(this line is not essential, but in the case when the config option is not recognized properly, this produces a regular test failure, instead of an exception due to a wrongly set up stub)

@stomar

This comment has been minimized.

Show comment
Hide comment
@stomar

stomar Jul 7, 2016

Contributor

Test case added.

I'm not very familiar with the code base and even less with the test suite, though. Hope it's ok.

Contributor

stomar commented Jul 7, 2016

Test case added.

I'm not very familiar with the code base and even less with the test suite, though. Hope it's ok.

@stomar

This comment has been minimized.

Show comment
Hide comment
@stomar

stomar Jul 7, 2016

Contributor

Note: it seems there sometimes occur random test failures on Travis. My original commit had two unrelated failures, after pushing again all was fine.

Contributor

stomar commented Jul 7, 2016

Note: it seems there sometimes occur random test failures on Travis. My original commit had two unrelated failures, after pushing again all was fine.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 7, 2016

Member

This LGTM. @envygeeks want to take one more look?

Member

parkr commented Jul 7, 2016

This LGTM. @envygeeks want to take one more look?

Show outdated Hide outdated test/test_configuration.rb Outdated
Stringify configuration overrides before first use
This makes sure that overrides for Jekyll.configuration
all have string keys before their first use, particularly
also the "config" and "skip_config_files" options.
@stomar

This comment has been minimized.

Show comment
Hide comment
@stomar

stomar Jul 8, 2016

Contributor

@envygeeks Never mind the previous comment; applied your suggestion.

Fixed also the other occurrences of wahoo.dev with PR #5068 [merged].

Contributor

stomar commented Jul 8, 2016

@envygeeks Never mind the previous comment; applied your suggestion.

Fixed also the other occurrences of wahoo.dev with PR #5068 [merged].

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 15, 2016

Member

LGTM.

Member

parkr commented Jul 15, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 16, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jul 16, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit eb88aa3 into jekyll:master Jul 16, 2016

1 of 2 checks passed

jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Jul 16, 2016

jekyllbot added a commit that referenced this pull request Jul 16, 2016

@stomar stomar deleted the stomar:stringify-overrides branch Jul 16, 2016

stevecheckoway added a commit to stevecheckoway/jekyll that referenced this pull request Jul 24, 2016

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