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

watcher: pass site instance to watch plugin #5109

Merged
merged 1 commit into from Jul 21, 2016

Conversation

Projects
None yet
4 participants
@mojavelinux
Contributor

mojavelinux commented Jul 18, 2016

  • prevents the watch plugin from creating a new site instance

This change won't take effect until jekyll/jekyll-watch#40 is incorporated into a release of the jekyll-watch gem.

Show outdated Hide outdated lib/jekyll/commands/build.rb
@@ -72,7 +72,8 @@ def build(site, options)
# Returns nothing.
def watch(_site, options)
External.require_with_graceful_fail "jekyll-watch"
Jekyll::Watcher.watch(options)
(watch = Jekyll::Watcher.method(:watch)).parameters.size == 1 ?
watch.call(options) : watch.call(options, _site)

This comment has been minimized.

@parkr

parkr Jul 19, 2016

Member

Hey @mojavelinux! We get a lot of Ruby newcomers and this syntax, while nicely concise, might be unwelcoming. Would you consider refactoring it? Very minor refactor, something like:

watch = Jekyll::Watcher.method(:watch)
if watch.parameters.size == 1
  watch.call(options)
else
  watch.call(options, site)
end

We should also move _site to site now that we're using the argument (a Rubocop thing).

@parkr

parkr Jul 19, 2016

Member

Hey @mojavelinux! We get a lot of Ruby newcomers and this syntax, while nicely concise, might be unwelcoming. Would you consider refactoring it? Very minor refactor, something like:

watch = Jekyll::Watcher.method(:watch)
if watch.parameters.size == 1
  watch.call(options)
else
  watch.call(options, site)
end

We should also move _site to site now that we're using the argument (a Rubocop thing).

This comment has been minimized.

@mojavelinux

mojavelinux Jul 19, 2016

Contributor

Gladly! Stay tuned.

@mojavelinux

mojavelinux Jul 19, 2016

Contributor

Gladly! Stay tuned.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 19, 2016

Member

Thank you!

Member

parkr commented Jul 19, 2016

Thank you!

@parkr parkr added the enhancement label Jul 19, 2016

@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Jul 19, 2016

Contributor

PR updated!

Contributor

mojavelinux commented Jul 19, 2016

PR updated!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Jul 19, 2016

Contributor

LGTM.

Contributor

envygeeks commented Jul 19, 2016

LGTM.

Show outdated Hide outdated lib/jekyll/commands/build.rb
External.require_with_graceful_fail "jekyll-watch"
Jekyll::Watcher.watch(options)
def watch(site, options)
External.require_with_graceful_fail 'jekyll-watch'

This comment has been minimized.

@parkr

parkr Jul 19, 2016

Member

Hey! One more thing: the travis build failed for this PR because you're using single quotes here. Our styleguide requests double quotes for all string literals. If you run script/fmt locally, you should see this as well. 😄

@parkr

parkr Jul 19, 2016

Member

Hey! One more thing: the travis build failed for this PR because you're using single quotes here. Our styleguide requests double quotes for all string literals. If you run script/fmt locally, you should see this as well. 😄

resolves #4858 pass site instance to watch plugin
- prevents the watch plugin from creating a new site instance
@mojavelinux

This comment has been minimized.

Show comment
Hide comment
@mojavelinux

mojavelinux Jul 20, 2016

Contributor

Fixed!

Contributor

mojavelinux commented Jul 20, 2016

Fixed!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 20, 2016

Member

LGTM. Thank you so much!!

Member

parkr commented Jul 20, 2016

LGTM. Thank you so much!!

@parkr parkr changed the title from resolves #4858 pass site instance to watch plugin to watcher: pass site instance to watch plugin Jul 21, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 21, 2016

Member

Fixes #4858.

@jekyllbot: merge +minor

Member

parkr commented Jul 21, 2016

Fixes #4858.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit f8aea7b into jekyll:master Jul 21, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Jul 21, 2016

stevecheckoway added a commit to stevecheckoway/jekyll that referenced this pull request Jul 24, 2016

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