Allow provide hostname to bind server to #719

Closed
wants to merge 7 commits into
from

Projects

None yet

4 participants

@ixti
Member
ixti commented Dec 29, 2012

Refactored variant of #686

@parkr
Member
parkr commented Dec 30, 2012

Nice work, @ixti, looks great.

@ixti
Member
ixti commented Dec 31, 2012

I'm going refactor this pull request and then re-open issue.

@ixti ixti closed this Dec 31, 2012
@ixti ixti Refactor address parsing
- Extract code into separate method
- Cover with tests
- Improve hostname/IP validation
c0cf3d5
@ixti ixti reopened this Jan 2, 2013
@ixti
Member
ixti commented Jan 2, 2013

I hope it's good to go now :D

@mattr-
Member
mattr- commented Jan 2, 2013

👍 one more small doc change to make, and then :shipit:

@ixti
Member
ixti commented Jan 3, 2013

/cc @mojombo I believe now it's really OK to be merged in :D

@mojombo mojombo and 2 others commented on an outdated diff Jan 4, 2013
bin/jekyll
options['server'] = true
- options['server_port'] = port unless port.nil?
+
+ unless addr.nil? or addr.empty?
@mojombo
mojombo Jan 4, 2013 collaborator

I'd prefer to see this as an if statement. It's hard to mentally parse unless statements like this.

@ixti
ixti Jan 4, 2013 Jekyll member

OK. I will change it to be:

if addr and not addr.empty?
@parkr
parkr Jan 4, 2013 Jekyll member

I think Tom will insist that the and keyword will not exist in this codebase:

if addr && not addr.empty?
@ixti
ixti Jan 4, 2013 Jekyll member

Fixed to be:

if addr && !addr.empty?
@mojombo mojombo and 3 others commented on an outdated diff Jan 4, 2013
lib/jekyll.rb
+ host = nil if host.empty?
+ else
+ host, port = nil, addr
+ end
+
+ raise "Invalid host: '#{host}' (in: '#{addr}')" unless valid_host? host
+ raise "Invalid port: '#{port}' (in: '#{addr}')" unless valid_port? port
+
+ [host, port.to_i]
+ end
+
+ protected
+
+ # Internal: Test validity of given port
+ def self.valid_port?(port)
+ 0 < port.to_i
@mojombo
mojombo Jan 4, 2013 collaborator

Strange to pose it this way. Why not port.to_i > 0?

@ixti
ixti Jan 4, 2013 Jekyll member

If it's an issue - I'll change it. It's just my habit to write conditions in Yoda style (thanks @parkr - never knew how to refer this style). For me it seems easier to parse mentally this style:

if 0 < (port = server.port_number)
# vs
if (port = server.port_number) > 0

The reason behind is that it's easier to remember a constant that is going to be compared with expression than to recall what expression might evaluate upon comparing it to constant. And it makes sure that you'll get an error if you'll mistype == condition an will write if 5 = port accidentally.

PS But again, it's just my habit. And all this "goodies" might be only in my head :D

@mattr-
mattr- Jan 4, 2013 Jekyll member

@ixti must have been a grizzled C programmer before getting into Ruby. 😉

@ixti
ixti Jan 4, 2013 Jekyll member

@mattr- PHP in fact 😄

@ixti
Member
ixti commented Jan 4, 2013

@mojombo fixed according to your wishes. :D

@parkr
Member
parkr commented Jan 4, 2013

👍

@ixti ixti Fix condition according to GH styleguides
See: https://github.com/styleguide/ruby

> The and and or keywords are banned.
> It's just not worth it. Always use && and || instead
6ca77b9
@parkr
Member
parkr commented Jan 11, 2013

@ixti What if someone wants to change just the host, but is fine with binding to the default port? This is a great idea, but after some more thought and discussion, it's not as "free" as we'd like. We're going to favour the --host and --port options to separate these two arguments out. Thanks for your hard work, though. :)

@parkr parkr closed this Jan 11, 2013
@ixti
Member
ixti commented Jan 11, 2013

Well it's up to you, but I can't imagine situation when you want specify host, and leave default port. It's just unusual.

@parkr parkr reopened this Jan 12, 2013
@parkr
Member
parkr commented Jan 12, 2013

@ixti I've re-read the comment string in #686 and am going to think on it further. I can think of a reason to change just the host and not the port (using a different .dev URL or something instead of different ports on localhost when serving several sites simultaneously) but that's the only use-case I can see which would prefer separate options. If you can think of any other use-cases which might convince me of one or the other, let me know. I like how this is backwards-compatible with the current behaviour and that it follows the WWW standard of HOST:PORT.

@ixti
Member
ixti commented Jan 14, 2013

I can't find any usage when you want to change host part that often while keeping port number the same. Your use IMO is way too unusual. You can't bind same ip/port twice, that means that you'll need to add different records into your hosts file or local DNS server in order to be allowed to run multiple hosts on same machine (you'll need multiple IPs for this).

After all I really don't see any problems explicitly specify default port if you want other than default host only:

  • it will be anyway shorter than adding --host argument prefix
  • it's really rare use case
@ixti
Member
ixti commented Jan 31, 2013

Looks like this PR is not relevant anymore.
The only thing I would like to mention is that default '0.0.0.0' is not a good choice IMHO. Host should be nil by default so it will bind to :: (IPv6) and 0.0.0.0 (IPv4)

@ixti ixti closed this Jan 31, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment