Add bundle install to jekyll new command #5237

Merged
merged 2 commits into from Sep 20, 2016

Projects

None yet

7 participants

@ashmaroli
Contributor
ashmaroli commented Aug 12, 2016 edited

This PR proposes to extend the jekyll new command with following:

  • Automatically cd my_blog and initiate bundle install

After you run jekyll new my_blog the command shell automatically cd into the new directory and runs bundle install.
If bundler is not installed, an exception is raised gracefully.
The user may choose to skip 'bundle install' if they plan to edit their new gemfile and change the theme gem, or if they want to inspect the new directory before running the bundle install command by using the new switch --skip-bundle

ref: #5225

@envygeeks envygeeks and 1 other commented on an outdated diff Aug 12, 2016
lib/jekyll/commands/new.rb
@@ -114,6 +116,30 @@ def site_template
def scaffold_path
"_posts/0000-00-00-welcome-to-jekyll.markdown.erb"
end
+
+ def bundle_install(path)
+ Jekyll::External.require_with_graceful_fail "bundler"
+ $stdout.print "\nRun 'bundle install' with default theme? (Y/N) "
@envygeeks
envygeeks Aug 12, 2016 Member

Please use the Jekyll Logger.

@ashmaroli
ashmaroli Aug 12, 2016 edited Contributor

Jekyll logger inserts a newline at the end.. I proposed print to have gets on same line.
Is that fine?

@envygeeks envygeeks commented on an outdated diff Aug 12, 2016
lib/jekyll/commands/new.rb
@@ -114,6 +116,30 @@ def site_template
def scaffold_path
"_posts/0000-00-00-welcome-to-jekyll.markdown.erb"
end
+
+ def bundle_install(path)
+ Jekyll::External.require_with_graceful_fail "bundler"
+ $stdout.print "\nRun 'bundle install' with default theme? (Y/N) "
+ @ans = ENV["CI"] ? "N" : $stdin.gets.chomp
+ until @ans == "Y" || @ans == "N"
@envygeeks
envygeeks Aug 12, 2016 Member

A standard CLI must understand all cases of "y" and "yes" and "n" and "no", "y/n" is not good enough here. I often type yes, rather than y, I am a very explicit person in what my intentions are, I'm not a keyboard basher. I would be fuming if it rejected "yes" and probably start a Twitter storm.

@envygeeks envygeeks and 1 other commented on an outdated diff Aug 12, 2016
lib/jekyll/commands/new.rb
+ @ans = ENV["CI"] ? "N" : $stdin.gets.chomp
+ until @ans == "Y" || @ans == "N"
+ $stdout.print "Invalid input. Input 'Y' or 'N' "
+ @ans = $stdin.gets.chomp
+ end
+ stdinput(path)
+ end
+
+ def stdinput(path)
+ if @ans == "Y"
+ Jekyll.logger.info "Running bundle install in", path.to_s + "..."
+ Dir.chdir(path.to_s) do
+ system("bundle install")
+ end
+ elsif @ans == "N"
+ Jekyll.logger.warn "\nBundle install skipped.\n" \
@envygeeks
envygeeks Aug 12, 2016 Member

This shouldn't be a warning. It's a valid choice.

@ashmaroli
ashmaroli Aug 12, 2016 Contributor

I added it as a warning as bundle install is a necessary step, yet not mandatory at that very instant.

@envygeeks
envygeeks Aug 12, 2016 Member

It shouldn't be a warning no matter how you phrase it. It's the users choice when they install. It should simply say "Skipping Bundle Install: You should do it later" or something to that extend. Shaming the user for making a choice makes them feel like your choice is the only choice. A user will do things their way, in their order.

@envygeeks
envygeeks Aug 12, 2016 Member

And that "Skipping bundle install" should be an informational message, not a warning.

@envygeeks envygeeks and 1 other commented on an outdated diff Aug 12, 2016
lib/jekyll/commands/new.rb
+ until @ans == "Y" || @ans == "N"
+ $stdout.print "Invalid input. Input 'Y' or 'N' "
+ @ans = $stdin.gets.chomp
+ end
+ stdinput(path)
+ end
+
+ def stdinput(path)
+ if @ans == "Y"
+ Jekyll.logger.info "Running bundle install in", path.to_s + "..."
+ Dir.chdir(path.to_s) do
+ system("bundle install")
+ end
+ elsif @ans == "N"
+ Jekyll.logger.warn "\nBundle install skipped.\n" \
+ "Edit your Gemfile and run 'bundle install' " \
@envygeeks
envygeeks Aug 12, 2016 Member

I don't know that this is useful tbh, any reason why this is here?

@ashmaroli
ashmaroli Aug 12, 2016 Contributor

skipping b i may not install minima for first-time users. They wont be able to jekyll serve till minima is found in the gemfile.lock

@envygeeks
envygeeks Aug 12, 2016 Member

Technically they won't be able to do anything Jekyll until they bundle install. This should be obvious though I will concede to new users it might not be obvious, so I don't disagree if you choose to leave it because I can see it's benefits.

@ashmaroli
Contributor

Add bundler as a runtime dependency to gemspec?

@envygeeks
Member

I don't think that's a good idea, that could lead to problems on a system.

@ashmaroli ashmaroli referenced this pull request Aug 21, 2016
Closed

Add --classic switch to 'jekyll new' command #5217

1 of 1 task complete
@parkr
Member
parkr commented Aug 22, 2016

Thanks for this PR! I'm not sure how I feel about this either. The current code is interactive, which I'm not such a big fan of. rails new always ran bundle install which I liked as a newcomer, but the Rails community is pretty different from the Jekyll community, I think. Not sure how I feel about this.

In general, we are headed in the direction of "always use bundler" so this isn't problematic from that perspective. But we should gracefully fail if bundler isn't available and mention that the user should run bundle install before running jekyll. 😄

@ashmaroli
Contributor

Hi @parkr, I'm pretty confused..

But we should gracefully fail if bundler isn't available

PR does that already..

mention that the user should run bundle install before running jekyll.

??
There wont be a Gemfile to run bundle against until jekyll new has been called.
Running bundle after blog install will install minima theme gem for first time users; update minima if necessary for existing customers on J3.2.
Running bundle interactively lets the user edit their Gemfile / view the installed directory contents before having a request sent to rubygems.org. Once they make their decision, the new theme has to be bundled before being served.

@ashmaroli ashmaroli referenced this pull request Sep 9, 2016
Open

Jekyll's Philosophy #5342

@mattr-
Member
mattr- commented Sep 9, 2016

I'm 👍 with the idea of running bundler when you create a new Jekyll site. Let's just run it all the time though, instead of asking the user if we should run it. I see no use case where a user would not want to run bundler after getting a new site.

@ashmaroli
Contributor

@mattr- Thanks for the +1. 😃

I see no use case where a user would not want to run bundler after getting a new site.

I mentioned a possible scenario in the PR desc.:
The user may choose to skip 'bundle install' if they plan to edit their new gemfile and change the theme gem, or if they want to inspect the new directory before running the bundle install command

@mattr-
Member
mattr- commented Sep 9, 2016

Those are technical use cases that I don't want to solve for, especially the directory inspection. While I expect that a large set of Jekyll's users are going to be on the more technical side, it's ridiculously easy to run bundle after we've done it as part of site generation after editing the Gemfile. I see where you're coming from on this, but I feel like it's only going to be used in a tiny set of cases and I'd rather come back and add it later if it's a highly requested feature.

@ashmaroli
Contributor

Okay. I'll modify the PR.

@ghost
ghost commented Sep 9, 2016

I'd prefer the simple option of an argument --no-bundler, there's no need to have an interactive prompt, Jekyll doesn't do this already for anything AFAIK. Other that that, 👍!

@ashmaroli
Contributor
ashmaroli commented Sep 12, 2016 edited

Replaced interactive bundle with running bundle install by default. Can be skipped by using a switch
--skip_bundle

@ashmaroli ashmaroli and 3 others commented on an outdated diff Sep 12, 2016
lib/jekyll/commands/new.rb
+
+ def after_install(path, options = {})
+ Jekyll.logger.info "New jekyll site installed in #{path.cyan}."
+ Jekyll.logger.info "Bundle install skipped." if options["skip_bundle"]
+
+ unless options["blank"] || options["skip_bundle"]
+ bundle_install path
+ end
+ end
+
+ def bundle_install(path)
+ Jekyll::External.require_with_graceful_fail "bundler"
+ Jekyll.logger.info "Running bundle install in #{path.cyan}..."
+ Dir.chdir(path) do
+ unless ENV["CI"] || ENV["TEST"] # skip `bundle` during local tests
+ system("bundle install")
@ashmaroli
ashmaroli Sep 12, 2016 edited Contributor

I'd like to skip bundle install when running tests. Couldn't find the appropriate ENV key/value..

@envygeeks
envygeeks Sep 12, 2016 Member

I don't know about all that but that should be system("bundle", "install")

@envygeeks
envygeeks Sep 12, 2016 Member

Meh, actually, tbh, it doesn't matter. I mix the two anyways I wouldn't blame you if you don't care to change it either.

@ghost
ghost Sep 12, 2016

Doesn't really matter until you start passing arguments dynamically 😉 Although I do prefer the variadic argument form

@ashmaroli
ashmaroli Sep 12, 2016 Contributor

it works.. so leaving it as is..
Thanks for the info, anyways 😃

@envygeeks
envygeeks Sep 12, 2016 Member

@spudowiar it's that kind of terrible logic that leads to security issues in software. It always matters. Whether it works or not is irrelevant to the point, the statement stands and the option was left to leave it as is out of ease, not out of it "not mattering."

@ghost
ghost Sep 12, 2016

@envygeeks I'm saying that although you should always use the variadic form, in this case not changing it doesn't matter too much. I always use the variadic form

@parkr
parkr Sep 12, 2016 Member

I would prefer system("bundle", "install") as well. You may also have to specify the BUNDLER_GEMFILE variable as well.

@parkr parkr commented on an outdated diff Sep 12, 2016
lib/jekyll/commands/new.rb
@@ -11,6 +11,7 @@ def init_with_program(prog)
c.option "force", "--force", "Force creation even if PATH already exists"
c.option "blank", "--blank", "Creates scaffolding but with empty files"
+ c.option "skip_bundle", "--skip_bundle", "Skip 'bundle install'"
@parkr
parkr Sep 12, 2016 Member

I would prefer --skip-bundle here. Consistency of hyphens seems more intuitive.

@parkr parkr and 1 other commented on an outdated diff Sep 12, 2016
lib/jekyll/commands/new.rb
+ # unless the user opts to generate a blank blog or skip 'bundle install'.
+
+ def after_install(path, options = {})
+ Jekyll.logger.info "New jekyll site installed in #{path.cyan}."
+ Jekyll.logger.info "Bundle install skipped." if options["skip_bundle"]
+
+ unless options["blank"] || options["skip_bundle"]
+ bundle_install path
+ end
+ end
+
+ def bundle_install(path)
+ Jekyll::External.require_with_graceful_fail "bundler"
+ Jekyll.logger.info "Running bundle install in #{path.cyan}..."
+ Dir.chdir(path) do
+ unless ENV["CI"] || ENV["TEST"] # skip `bundle` during local tests
@parkr
parkr Sep 12, 2016 Member

I think we should be testing all our functionality in the tests. Ideally we'd just pass --skip-bundle if we really didn't want to bundle in CI.

@ghost
ghost Sep 12, 2016

I agree with @parkr, running bundle install won't hurt in tests, we can always skip it if time is precious

ashmaroli added some commits Aug 12, 2016
@ashmaroli ashmaroli add `bundle install` to `jekyll new`
 - automatically run `bundle install` from within the newly generated blog
   directory by default.
 - add a new switch to skip this default behaviour.
566b42b
@ashmaroli ashmaroli test bundle install and skipping it.
345f043
@ashmaroli
Contributor

@jekyll/ecosystem : Since, this PR adds bundle install by default, should script/default-site be modified to avoid switching directories and then executing bundle install, as well?

@benbalter benbalter was assigned by jekyllbot Sep 13, 2016
@ashmaroli
Contributor

Updates:

  • changed skip switch to --skip-bundle
  • system(bundle install) replaced with system("bundle", "install")
  • modified test/test_new_command to test bundle-install-action and skip-switch
@ghost Unknown commented on the diff Sep 13, 2016
lib/jekyll/commands/new.rb
@@ -114,6 +115,27 @@ def site_template
def scaffold_path
"_posts/0000-00-00-welcome-to-jekyll.markdown.erb"
end
+
+ # After a new blog has been created, print a success notification and
+ # then automatically execute bundle install from within the new blog dir
+ # unless the user opts to generate a blank blog or skip 'bundle install'.
+
+ def after_install(path, options = {})
+ Jekyll.logger.info "New jekyll site installed in #{path.cyan}."
+ Jekyll.logger.info "Bundle install skipped." if options["skip-bundle"]
@ghost
ghost Sep 13, 2016

Seems superfluous since --skip-bundle is an explicitly chosen command line option, unaffected by the environment in any way

@parkr
parkr approved these changes Sep 16, 2016 View changes

This LGTM. Would love another review from someone in @jekyll/core or @benbalter.

@benbalter
Contributor

LGTM.

@parkr
Member
parkr commented Sep 20, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 504411e into jekyll:master Sep 20, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Awaiting approval from at least 2 maintainers.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@parkr parkr added this to the 3.3 milestone Sep 20, 2016
@ashmaroli
Contributor
ashmaroli commented Sep 21, 2016 edited

TODO:

  • Update online documentation regarding this new feature. :
    • add a para under Quickstart

/cc @DirtyF
Whatsay? Is there any other place I should update?
Or will it be updated alongwith the Release Notes for 3.3?

@ashmaroli
Contributor

@parkr et al, All those bundle (show) reports in test logs might get frustrating at some point in future..
Your opinion?

@parkr
Member
parkr commented Sep 21, 2016

All those bundle (show) reports in test logs might get frustrating at some point in future..
Your opinion?

Yeah definitely. If you can capture those and output them using Jekyll.logger then our current capture_stdout should ensure they're not written to actual stdout.

@DirtyF
Member
DirtyF commented Sep 21, 2016

@ashmaroli yeah, homepage snippet will also be affected by this change, be sure to reflect changes here too.

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