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

set site.url in dev environment to `http://localhost:4000` #5431

Merged
merged 2 commits into from Oct 5, 2016

Conversation

Projects
None yet
6 participants
@Crunch09
Member

Crunch09 commented Sep 30, 2016

No description provided.

@Crunch09 Crunch09 referenced this pull request Sep 30, 2016

Closed

Jekyll 3.3 Release Gameplan #5400

9 of 9 tasks complete
@parkr

Excellent, thank you for picking this up. Just a few quick comments.

Show outdated Hide outdated test/test_commands_serve.rb
allow(Jekyll::Commands::Build).to receive(:build).and_return("")
end
teardown do
Jekyll.instance_variable_set(:@sites, [])

This comment has been minimized.

@parkr

parkr Sep 30, 2016

Member

You should be able to do Jekyll.sites.clear here.

@parkr

parkr Sep 30, 2016

Member

You should be able to do Jekyll.sites.clear here.

This comment has been minimized.

@Crunch09

Crunch09 Oct 1, 2016

Member

🙈 thanks!

@Crunch09

Crunch09 Oct 1, 2016

Member

🙈 thanks!

Show outdated Hide outdated lib/jekyll/commands/serve.rb
@@ -33,6 +33,8 @@ def init_with_program(prog)
opts["serving"] = true
opts["watch" ] = true unless opts.key?("watch")
config = opts["config"]
default_url = "http://localhost:4000"

This comment has been minimized.

@parkr

parkr Sep 30, 2016

Member

One can set the host and port options in the command line or via your config file. If I set my --port to 3999, I should get my default url as http://localhost:3999. Same with --host.

@parkr

parkr Sep 30, 2016

Member

One can set the host and port options in the command line or via your config file. If I set my --port to 3999, I should get my default url as http://localhost:3999. Same with --host.

This comment has been minimized.

@Crunch09

Crunch09 Oct 1, 2016

Member

@parkr Thanks! Should i also take baseurl and ssl options into consideration?

@Crunch09

Crunch09 Oct 1, 2016

Member

@parkr Thanks! Should i also take baseurl and ssl options into consideration?

This comment has been minimized.

@parkr

parkr Oct 1, 2016

Member

ssl yes, baseurl nope!

@parkr

parkr Oct 1, 2016

Member

ssl yes, baseurl nope!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 2, 2016

Contributor

I'm confused as to what is going on here? We already do all this? See: https://github.com/jekyll/jekyll/blob/master/lib/jekyll/commands/serve.rb#L119 that has been in there for quite some time and should automatically change itself based on everything in the options.

Contributor

envygeeks commented Oct 2, 2016

I'm confused as to what is going on here? We already do all this? See: https://github.com/jekyll/jekyll/blob/master/lib/jekyll/commands/serve.rb#L119 that has been in there for quite some time and should automatically change itself based on everything in the options.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 2, 2016

Contributor

Oh, I see what's going on, okay, yeah, can you just use our helper method that will generate everything for you and put it onto site? That would make your life much easier @Crunch09!

Contributor

envygeeks commented Oct 2, 2016

Oh, I see what's going on, okay, yeah, can you just use our helper method that will generate everything for you and put it onto site? That would make your life much easier @Crunch09!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Oct 2, 2016

Contributor

Oh, you need baseurl not to be there as per @parkr's specs, you can adjust that method to exclude it if you want, add a conditional into the params and then use that conditional in the builder and you can exclude it!

Contributor

envygeeks commented Oct 2, 2016

Oh, you need baseurl not to be there as per @parkr's specs, you can adjust that method to exclude it if you want, add a conditional into the params and then use that conditional in the builder and you can exclude it!

@parkr parkr added this to the 3.3 milestone Oct 3, 2016

@parkr

One more tiny nit then it's 👍 !!!

Show outdated Hide outdated lib/jekyll/commands/serve.rb
@@ -33,6 +33,8 @@ def init_with_program(prog)
opts["serving"] = true
opts["watch" ] = true unless opts.key?("watch")
config = opts["config"]
url = default_url(opts)
opts = opts.merge({ "url" => url }) if Jekyll.env == "development"

This comment has been minimized.

@parkr

parkr Oct 3, 2016

Member

Let's avoid the url variable assignment here and just do

opts.merge!({ "url" => default_url(opts) }) if Jekyll.env == "development"
@parkr

parkr Oct 3, 2016

Member

Let's avoid the url variable assignment here and just do

opts.merge!({ "url" => default_url(opts) }) if Jekyll.env == "development"

This comment has been minimized.

@Crunch09

Crunch09 Oct 4, 2016

Member

Ah thanks, i only tried it with the non-bang version and then the line was too long :)

@Crunch09

Crunch09 Oct 4, 2016

Member

Ah thanks, i only tried it with the non-bang version and then the line was too long :)

This comment has been minimized.

@parkr

parkr Oct 4, 2016

Member

Rubocop corrected me further:

opts["url"] = default_url(opts) if Jekyll.env == "development"
@parkr

parkr Oct 4, 2016

Member

Rubocop corrected me further:

opts["url"] = default_url(opts) if Jekyll.env == "development"

This comment has been minimized.

@Crunch09

Crunch09 Oct 4, 2016

Member

Thanks, just wanted to post the same thing 😆 Updated commit coming in a few minutes

@Crunch09

Crunch09 Oct 4, 2016

Member

Thanks, just wanted to post the same thing 😆 Updated commit coming in a few minutes

@@ -79,16 +85,53 @@ def custom_opts(what)
custom_options = {
"config" => %w(_config.yml _development.yml),
"serving" => true,
"watch" => false # for not having guard output when running the tests
"watch" => false, # for not having guard output when running the tests
"url" => "http://localhost:4000"

This comment has been minimized.

@parkr

parkr Oct 3, 2016

Member

Why is this added?

@parkr

parkr Oct 3, 2016

Member

Why is this added?

This comment has been minimized.

@Crunch09

Crunch09 Oct 4, 2016

Member

This test checks which options are passed to Serve.process and the URL is now also included in that options hash.

@Crunch09

Crunch09 Oct 4, 2016

Member

This test checks which options are passed to Serve.process and the URL is now also included in that options hash.

@DirtyF DirtyF referenced this pull request Oct 4, 2016

Closed

localhost website pointing to remote git repository #327

3 of 4 tasks complete

Crunch09 added some commits Sep 30, 2016

default site.url in dev environment to `http://localhost:4000`
take `host`, `port` and `ssl` options into account
@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Oct 4, 2016

Member

@envygeeks I refactored it now to also use the server_address method.

Member

Crunch09 commented Oct 4, 2016

@envygeeks I refactored it now to also use the server_address method.

@parkr

parkr approved these changes Oct 5, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 5, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Oct 5, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit d879840 into jekyll:master Oct 5, 2016

1 of 2 checks passed

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

jekyllbot added a commit that referenced this pull request Oct 5, 2016

@Crunch09 Crunch09 deleted the Crunch09:update_url_in_dev_environment branch Oct 6, 2016

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Oct 6, 2016

Contributor

Just to note, and this doesn't need to hold up 3.3., that we went from passing server and a hash of named arguments, to passing four unnamed arguments to server_address (meaning every option we add now has to be an argument). I'd think passing the options hash around, rather than the individual arguments would be a bit cleaner and future proof.

(Noticed because of #5451).

Contributor

benbalter commented Oct 6, 2016

Just to note, and this doesn't need to hold up 3.3., that we went from passing server and a hash of named arguments, to passing four unnamed arguments to server_address (meaning every option we add now has to be an argument). I'd think passing the options hash around, rather than the individual arguments would be a bit cleaner and future proof.

(Noticed because of #5451).

@Frikki

This comment has been minimized.

Show comment
Hide comment
@Frikki

Frikki Oct 6, 2016

Why wasn’t there a test suite showning this error #5451 when these changes were made?

Frikki commented Oct 6, 2016

Why wasn’t there a test suite showning this error #5451 when these changes were made?

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Oct 6, 2016

Contributor

Why wasn’t there a test suite showning this error #5451 when these changes were made?

Because JekyllAdmin is a plugin and distributed/versioned independently.

Contributor

benbalter commented Oct 6, 2016

Why wasn’t there a test suite showning this error #5451 when these changes were made?

Because JekyllAdmin is a plugin and distributed/versioned independently.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Oct 6, 2016

Member

I'd think passing the options hash around, rather than the individual arguments would be a bit cleaner and future proof.

@benbalter Yes, it probably would be more future proof to just use an option hash. But on the other hand i didn't think of any additional arguments we could pass to create a server address and it also just was a private method.

Member

Crunch09 commented Oct 6, 2016

I'd think passing the options hash around, rather than the individual arguments would be a bit cleaner and future proof.

@benbalter Yes, it probably would be more future proof to just use an option hash. But on the other hand i didn't think of any additional arguments we could pass to create a server address and it also just was a private method.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 6, 2016

Member

@benbalter @Crunch09 I think the best option forward is to revert the signature change, then. It was just for aesthetics AFAICT.

Member

parkr commented Oct 6, 2016

@benbalter @Crunch09 I think the best option forward is to revert the signature change, then. It was just for aesthetics AFAICT.

@Crunch09

This comment has been minimized.

Show comment
Hide comment
@Crunch09

Crunch09 Oct 6, 2016

Member

@parkr You're right, just to DRY things up. @benbalter

Member

Crunch09 commented Oct 6, 2016

@parkr You're right, just to DRY things up. @benbalter

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