`Serve.process` should receive same config as `Build.process` #4953

Merged
merged 1 commit into from Jun 2, 2016

Conversation

Projects
None yet
6 participants
@Crunch09
Member

Crunch09 commented May 26, 2016

Before:
bildschirmfoto 2016-05-27 um 00 01 43

After:
bildschirmfoto 2016-05-27 um 00 02 36

The config option is removed in the build process so it was not available for the serve command anymore. I tried to replace the delete in this line with a fetch at first but this was causing other tests to fail.

fixes #4850

lib/jekyll/commands/serve.rb
@@ -33,7 +33,9 @@ def init_with_program(prog)
cmd.action do |_, opts|
opts["serving"] = true
opts["watch" ] = true unless opts.key?("watch")
+ cfg = opts["config"]

This comment has been minimized.

@envygeeks

envygeeks May 26, 2016

Contributor

The short we normally use in Ruby is config.

@envygeeks

envygeeks May 26, 2016

Contributor

The short we normally use in Ruby is config.

lib/jekyll/commands/serve.rb
Build.process(opts)
+ opts["config"] = cfg

This comment has been minimized.

@envygeeks

envygeeks May 26, 2016

Contributor

Why does this need to happen? This is not only highly disoptimized by I don't understand why it has to happen and if it does I think there is a much bigger problem that needs to be addressed that either should be addressed here or this blocked on that being fixed.

@envygeeks

envygeeks May 26, 2016

Contributor

Why does this need to happen? This is not only highly disoptimized by I don't understand why it has to happen and if it does I think there is a much bigger problem that needs to be addressed that either should be addressed here or this blocked on that being fixed.

This comment has been minimized.

@Crunch09

Crunch09 May 26, 2016

Member

Thanks for the review! As i said in my initial comment the config option gets lost in the build process on this specific line and the serve call, which is started after the build process has finished has no information about the given configs anymore. I tried to replace the delete call with fetch but this was causing other tests to fail so i took another approach.

@Crunch09

Crunch09 May 26, 2016

Member

Thanks for the review! As i said in my initial comment the config option gets lost in the build process on this specific line and the serve call, which is started after the build process has finished has no information about the given configs anymore. I tried to replace the delete call with fetch but this was causing other tests to fail so i took another approach.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 27, 2016

Member

Your test has a formatting error! Would you mind fixing that up, please?

Member

parkr commented May 27, 2016

Your test has a formatting error! Would you mind fixing that up, please?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 May 27, 2016

Member

@parkr Of course! Sorry, i should've run the formatting tests locally first.

Member

Crunch09 commented May 27, 2016

@parkr Of course! Sorry, i should've run the formatting tests locally first.

test/test_commands_serve.rb
- 'config' => ['_config.yml', '_development.yml'],
- 'serving' => true,
- 'watch' => false # for not having guard output when running the tests
+ "config" => ["_config.yml", "_development.yml"],

This comment has been minimized.

@envygeeks

envygeeks May 27, 2016

Contributor

This should be %w(_config.yml _development.yml) This is a bug in our .rubocop.yml

@envygeeks

envygeeks May 27, 2016

Contributor

This should be %w(_config.yml _development.yml) This is a bug in our .rubocop.yml

This comment has been minimized.

@Crunch09

Crunch09 May 27, 2016

Member

Thanks!

@Crunch09

Crunch09 May 27, 2016

Member

Thanks!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 27, 2016

Contributor

Even though I don't like we have to reset the options, this is GTG. :shipit:

Contributor

envygeeks commented May 27, 2016

Even though I don't like we have to reset the options, this is GTG. :shipit:

@envygeeks envygeeks added the shipit label May 27, 2016

@tfe

This comment has been minimized.

Show comment
Hide comment
@tfe

tfe Jun 1, 2016

I'm not familiar with the Jekyll release process, but I've got some stuff dependent on this fix being merged. When do you think it'll make it into a release?

tfe commented Jun 1, 2016

I'm not familiar with the Jekyll release process, but I've got some stuff dependent on this fix being merged. When do you think it'll make it into a release?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 2, 2016

Member

@tfe: Merging now. Won't be in a release until v3.2.0.

@jekyllbot: merge +bug

Member

parkr commented Jun 2, 2016

@tfe: Merging now. Won't be in a release until v3.2.0.

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 6a07254 into jekyll:master Jun 2, 2016

1 check passed

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

@jekyllbot jekyllbot added bug fix labels Jun 2, 2016

jekyllbot added a commit that referenced this pull request Jun 2, 2016

@TWiStErRob

This comment has been minimized.

Show comment
Hide comment
@TWiStErRob

TWiStErRob Jul 5, 2016

Contributor

For anyone finding this issue/fix: I tested a lot of different versions and this broke between 3.0.3->3.0.4 and between 3.1.3->3.1.4. I discovered and researched this with baseurl not being picked up.

This means that the following versions cannot be used for local development properly:
Jekyll 3.0.4, 3.0.5 (pages v76-78), 3.1.4, 3.1.5 (pages v79), 3.1.6 (pages v80+)
and hence there's no stable version available at the moment that has this fix, what's the ETA for 3.2.0?

@envygeeks if you don't like resetting options, maybe there's a different fix clue hidden in those diffs.

Contributor

TWiStErRob commented Jul 5, 2016

For anyone finding this issue/fix: I tested a lot of different versions and this broke between 3.0.3->3.0.4 and between 3.1.3->3.1.4. I discovered and researched this with baseurl not being picked up.

This means that the following versions cannot be used for local development properly:
Jekyll 3.0.4, 3.0.5 (pages v76-78), 3.1.4, 3.1.5 (pages v79), 3.1.6 (pages v80+)
and hence there's no stable version available at the moment that has this fix, what's the ETA for 3.2.0?

@envygeeks if you don't like resetting options, maybe there's a different fix clue hidden in those diffs.

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