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

Detachment #1443

Merged
merged 6 commits into from Aug 25, 2013

Conversation

Projects
None yet
6 participants
@ddavison

ddavison commented Aug 21, 2013

No description provided.

@parkr

This comment has been minimized.

Member

parkr commented Aug 22, 2013

Awesome!

This should definitely come with instructions regarding killing the detached process though (other than restarting the computer). :)

@ddavison

This comment has been minimized.

ddavison commented Aug 22, 2013

haha! well the way it works is that it shows "webrick started.. pid=1234..port=4000" so you're able to fire a kill -9 .. on the pid but that's definitely something to add to the doc

@ddavison

This comment has been minimized.

ddavison commented Aug 22, 2013

done!

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

does this look good to go @parkr ?

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

Pretty much, yes! Let's just make sure @mattr- is interested as well. I like the feature myself.

Perhaps this detaching behaviour could be done before the build so that serving is immediate?

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

Would you also explicitly make the --detach flag correspond to a -d shortflag?

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

-d is reserved for the destination arg.. maybe another? pretty sure that's why -D for the Drafts wasn't using it.

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

Ah, of course. Shouldn't be suggesting things so late at night after so many early mornings :)

-D is taken by --drafts. What do you think?

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

I've noticed that several options dont even have short flags. maybe omit it? unless we can think of a better alternative. I can't at this moment!

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

how about just renaming the functionality to --fork and using the -f flag..doesn't appear to be taken. although i do like the term 'detach' ;)

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

Unfortunately if you omit it, Commander automatically creates one.

Maybe use -B or something for "background?"

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

I would still prefer --detatch, as it makes more sense semantically.

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

right. i got the fork term from the way that the mongoDB server works. mongod --fork runs in the background.

you don't see --background too often, do you?

so if you omit it, what flag does Commander create?

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

-B might be the best that we can agree on.. i'll go with that. 👍 i'll update the doc and the options. got a few minutes, then merge it in? or are you still waiting on mattr's consent?

@parkr

This comment has been minimized.

Member

parkr commented Aug 23, 2013

LGTM. I always wait on @mattr-'s consent unless it's something super simple.

Thanks for your work on this!

@ddavison

This comment has been minimized.

ddavison commented Aug 23, 2013

just doing my part ;)

i've been using jekyll for less than a week, and i like what i see.. only thing (that i noticed)
missing, was this!

@@ -15,6 +15,7 @@ class Configuration < Hash
'timezone' => nil, # use the local timezone
'safe' => false,
'detach' => false, # don't default to detaching the server

This comment has been minimized.

@mattr-

mattr- Aug 24, 2013

Member

This is really really nitpicky, but I like default to not detaching the server here, since it's a more positive type of comment.

trap("INT") { s.shutdown }
t.join()
unless options['detach']

This comment has been minimized.

@mattr-

mattr- Aug 24, 2013

Member

Let's reverse the conditional here to say:

if options['detach']
  pid = Process.fork { s.start }
  Process.detach(pid)
  pid
else
  t = Thread.new { s.start }
  trap("INT") { s.shutdown }
  t.join()
end

using an if here is easier to read through vs. an unless

bin/jekyll Outdated
@@ -82,6 +82,7 @@ command :serve do |c|
c.option '--limit_posts MAX_POSTS', Integer, 'Limits the number of posts to parse and publish'
c.option '-w', '--watch', 'Watch for changes and rebuild'
c.option '--lsi', 'Use LSI for improved related posts'
c.option '-B', '--detach', 'Run this server in the background (detach)'

This comment has been minimized.

@mattr-

mattr- Aug 24, 2013

Member

s/this/the

This comment has been minimized.

@ddavison

ddavison Aug 25, 2013

hey @mattr- not sure what you are getting at with this comment. could you clarify ?

This comment has been minimized.

@ixti

ixti Aug 25, 2013

Member

it's a sed script. means replace this with the

@ddavison

This comment has been minimized.

ddavison commented Aug 25, 2013

@mattr- changes implemented

@mattr-

This comment has been minimized.

Member

mattr- commented Aug 25, 2013

Nice work! Thanks! 😍

mattr- added a commit that referenced this pull request Aug 25, 2013

@mattr- mattr- merged commit 4fa4ad2 into jekyll:master Aug 25, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Aug 25, 2013

@ddavison ddavison deleted the ddavison:detachment branch Aug 25, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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