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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

create configuration from options only once in the boot process #5487

Merged
merged 5 commits into from Apr 19, 2017

Conversation

Projects
None yet
6 participants
@Crunch09
Member

Crunch09 commented Oct 16, 2016

Hi,

before, Jekyll.configuration was called 3 times during the serve command as described in #5467 . With this patch it is called only once:

screen shot 2016-10-16 at 22 48 22

I also made sure that jekyll-admin still works with this, to avoid the disaster from last time 馃槉 .

@jekyll/build

@DirtyF DirtyF added the fix label Oct 17, 2016

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 15, 2016

Member

whats the status on this?

Member

ashmaroli commented Nov 15, 2016

whats the status on this?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 15, 2016

Member

Hey @ashmaroli ,

because i edited my first post, i think jekyllbot didn't ping the correct team. So pinging again now 馃槅

/cc @jekyll/build

Member

Crunch09 commented Nov 15, 2016

Hey @ashmaroli ,

because i edited my first post, i think jekyllbot didn't ping the correct team. So pinging again now 馃槅

/cc @jekyll/build

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 15, 2016

Member

I kind of wish we could just make configuration_from_options idempotent 鈥 to have some guard that says if it's already a setup Jekyll::Configuration instance, then just return it.

def configuration_from_options(opts)
  return opts if opts.is_a?(Jekyll::Configuration) && opts.complete?

  # do more stuff
end

Then we'd pass along the completed Jekyll::Configuration instance.

Also, when we create a site, the site is added to Jekyll.sites. If a site already exists, we should just use that one so we don't recreate them. That already has the config on it!

Member

parkr commented Nov 15, 2016

I kind of wish we could just make configuration_from_options idempotent 鈥 to have some guard that says if it's already a setup Jekyll::Configuration instance, then just return it.

def configuration_from_options(opts)
  return opts if opts.is_a?(Jekyll::Configuration) && opts.complete?

  # do more stuff
end

Then we'd pass along the completed Jekyll::Configuration instance.

Also, when we create a site, the site is added to Jekyll.sites. If a site already exists, we should just use that one so we don't recreate them. That already has the config on it!

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Nov 15, 2016

Member

@parkr Thanks for your feedback, sounds like a good approach to me. I'll try to come up with a solution like this.

Member

Crunch09 commented Nov 15, 2016

@parkr Thanks for your feedback, sounds like a good approach to me. I'll try to come up with a solution like this.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Dec 1, 2016

Member

@parkr Hi, I updated the code as you suggested but left out the complete? check because i wasn't sure when a Configuration should be considered complete and also the test suite already passed (at least on minitest 5.9 馃檮 ).

Also, when we create a site, the site is added to Jekyll.sites. If a site already exists, we should just use that one so we don't recreate them. That already has the config on it!

I thought it would be best to create a separate PR to tackle this issue after this PR got merged?

Member

Crunch09 commented Dec 1, 2016

@parkr Hi, I updated the code as you suggested but left out the complete? check because i wasn't sure when a Configuration should be considered complete and also the test suite already passed (at least on minitest 5.9 馃檮 ).

Also, when we create a site, the site is added to Jekyll.sites. If a site already exists, we should just use that one so we don't recreate them. That already has the config on it!

I thought it would be best to create a separate PR to tackle this issue after this PR got merged?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Jan 10, 2017

Member

@parkr Could you take another look at this one if you have a few minutes, please?

Member

Crunch09 commented Jan 10, 2017

@parkr Could you take another look at this one if you have a few minutes, please?

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Apr 6, 2017

Member

@parkr Would be great if you could take another look at this one if you have time. Would love to see this getting merged. Let me know if there is anything missing.

Member

Crunch09 commented Apr 6, 2017

@parkr Would be great if you could take another look at this one if you have time. Would love to see this getting merged. Let me know if there is anything missing.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Apr 7, 2017

Member

@Crunch09 FWIW I tested your PR and it works fine by me.

Member

DirtyF commented Apr 7, 2017

@Crunch09 FWIW I tested your PR and it works fine by me.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Apr 7, 2017

Member

@DirtyF Great, happy to hear that :)

Member

Crunch09 commented Apr 7, 2017

@DirtyF Great, happy to hear that :)

@parkr

parkr approved these changes Apr 19, 2017

One suggestion to save a one-off method but other than that :shipit: if CI is 馃挌!

Show outdated Hide outdated lib/jekyll/commands/serve.rb

@DirtyF DirtyF requested a review from pathawks Apr 19, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Apr 19, 2017

Member

LGTM 馃憤

Member

pathawks commented Apr 19, 2017

LGTM 馃憤

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 19, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Apr 19, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 0108b22 into jekyll:master Apr 19, 2017

2 checks passed

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

jekyllbot added a commit that referenced this pull request Apr 19, 2017

@Crunch09 Crunch09 deleted the Crunch09:duplicate_config_reads branch Apr 19, 2017

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Apr 19, 2017

Member

Thanks!

Member

Crunch09 commented Apr 19, 2017

Thanks!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 19, 2017

Member

Good work @Crunch09!

Member

parkr commented Apr 19, 2017

Good work @Crunch09!

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