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

Write Jekyll::Utils::Exec.run for running shell commands. #5640

Merged
merged 6 commits into from Jan 18, 2017

Conversation

Projects
None yet
7 participants
@parkr
Member

parkr commented Dec 7, 2016

This fixes #5463 by taking @envygeeks's run_in_shell command and port it directly into the Jekyll codebase.

Any commands Jekyll has to run should be run with this method.

/cc @jekyll/core @ashmaroli

@parkr parkr added the fix label Dec 7, 2016

Show outdated Hide outdated features/support/helpers.rb
@@ -106,21 +105,16 @@ def run_jekyll(args)
# rubocop:disable Metrics/AbcSize

This comment has been minimized.

@mattr-

mattr- Dec 7, 2016

Member

can we remove the disabling of this check for this method now that it's been refactored?

@mattr-

mattr- Dec 7, 2016

Member

can we remove the disabling of this check for this method now that it's been refactored?

Show outdated Hide outdated lib/jekyll/commands/new.rb
system("bundle", "install", "--quiet")
else
system("bundle", "install")
_, output = Jekyll::Utils::Exec.run("bundle", "install", "--quiet")

This comment has been minimized.

@mattr-

mattr- Dec 7, 2016

Member

why do we not care about the return value here?

@mattr-

mattr- Dec 7, 2016

Member

why do we not care about the return value here?

This comment has been minimized.

@envygeeks

envygeeks Dec 7, 2016

Contributor

More importantly, why do you care about the output and not the status when you tell bundler to shut up while it's doing work?

@envygeeks

envygeeks Dec 7, 2016

Contributor

More importantly, why do you care about the output and not the status when you tell bundler to shut up while it's doing work?

parkr added some commits Dec 7, 2016

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Dec 8, 2016

Member

Updated.

Member

parkr commented Dec 8, 2016

Updated.

@mattr-

mattr- approved these changes Dec 8, 2016

@ashmaroli

Thank you for undertaking this @parkr ❤️.
Just a couple of queries..

unless options["blank"] || options["skip-bundle"]
bundle_install path
end
Jekyll.logger.info "New jekyll site installed in #{path.cyan}."
Jekyll.logger.info "Bundle install skipped." if options["skip-bundle"]
end

This comment has been minimized.

@ashmaroli

ashmaroli Dec 8, 2016

Member

out of curiosity, why was this moved to further ahead?
It was initially:

install directory > display success message > initiate 'bundle install'

now its:

install directory > initiate 'bundle install' > display install success message
@ashmaroli

ashmaroli Dec 8, 2016

Member

out of curiosity, why was this moved to further ahead?
It was initially:

install directory > display success message > initiate 'bundle install'

now its:

install directory > initiate 'bundle install' > display install success message

This comment has been minimized.

@ashmaroli

ashmaroli Dec 8, 2016

Member

This change here would warrant a change to the test file here

@ashmaroli

ashmaroli Dec 8, 2016

Member

This change here would warrant a change to the test file here

This comment has been minimized.

@pathawks

pathawks Dec 22, 2016

Member

Doesn't it seem like we should actually install Jekyll before we claim that Jekyll was installed?

We could add an additional message before bundle_install that the directory was setup, but is it worth it?

@pathawks

pathawks Dec 22, 2016

Member

Doesn't it seem like we should actually install Jekyll before we claim that Jekyll was installed?

We could add an additional message before bundle_install that the directory was setup, but is it worth it?

This comment has been minimized.

@ashmaroli

ashmaroli Dec 23, 2016

Member

Doesn't it seem like we should actually install Jekyll before we claim that Jekyll was installed?

One wouldn't be able to run jekyll new if Jekyll wasn't installed already. 😁
(I get it that was not your point.. 😀 )

By the time install-success message is displayed, the new project site would have already been created at the path specified.
The user is then informed of the initiation of the automated bundle install .. (which runs in context to the newly generated Gemfile)

But like I mentioned in the previous comment, if Parker decides to stick with his vision, he'll have to edit the test file accordingly as well.

@ashmaroli

ashmaroli Dec 23, 2016

Member

Doesn't it seem like we should actually install Jekyll before we claim that Jekyll was installed?

One wouldn't be able to run jekyll new if Jekyll wasn't installed already. 😁
(I get it that was not your point.. 😀 )

By the time install-success message is displayed, the new project site would have already been created at the path specified.
The user is then informed of the initiation of the automated bundle install .. (which runs in context to the newly generated Gemfile)

But like I mentioned in the previous comment, if Parker decides to stick with his vision, he'll have to edit the test file accordingly as well.

system("bundle", "install")
process, output = Jekyll::Utils::Exec.run("bundle", "install")
output.to_s.each_line do |line|
Jekyll.logger.info("Bundler:".green, line.strip) unless line.to_s.empty?

This comment has been minimized.

@ashmaroli

ashmaroli Dec 8, 2016

Member

I'm always 👍 for a little color out there.. but is Bundler: really necessary? (Though it doesn't hurt to be extra descriptive..)

@ashmaroli

ashmaroli Dec 8, 2016

Member

I'm always 👍 for a little color out there.. but is Bundler: really necessary? (Though it doesn't hurt to be extra descriptive..)

This comment has been minimized.

@pathawks

pathawks Dec 10, 2016

Member

but is Bundler: really necessary?

Yes. Nice to know where the output is coming from.

@pathawks

pathawks Dec 10, 2016

Member

but is Bundler: really necessary?

Yes. Nice to know where the output is coming from.

This comment has been minimized.

@DirtyF

DirtyF Jan 6, 2017

Member

Should not we colorize the output rather than Bundler:?

jekyll-new-output

@DirtyF

DirtyF Jan 6, 2017

Member

Should not we colorize the output rather than Bundler:?

jekyll-new-output

This comment has been minimized.

@ashmaroli

ashmaroli Jan 6, 2017

Member

I like how it is in Before.. In After, I need to consciously ignore the Bundler: .. but thats just my nit.....

@ashmaroli

ashmaroli Jan 6, 2017

Member

I like how it is in Before.. In After, I need to consciously ignore the Bundler: .. but thats just my nit.....

Show outdated Hide outdated features/support/helpers.rb
end
p.value
p
end
# rubocop:enable Metrics/AbcSize

This comment has been minimized.

@pathawks

pathawks Jan 6, 2017

Member

Since we removed rubocop:disable, we could probably remove rubocop:enable as well

@pathawks

pathawks Jan 6, 2017

Member

Since we removed rubocop:disable, we could probably remove rubocop:enable as well

pathawks added some commits Jan 6, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 6, 2017

Member

Updated the tests, as prescribed by @ashmaroli 👍

Member

pathawks commented Jan 6, 2017

Updated the tests, as prescribed by @ashmaroli 👍

@DirtyF

👍 Works fine. Just a UX feedback.

system("bundle", "install")
process, output = Jekyll::Utils::Exec.run("bundle", "install")
output.to_s.each_line do |line|
Jekyll.logger.info("Bundler:".green, line.strip) unless line.to_s.empty?

This comment has been minimized.

@DirtyF

DirtyF Jan 6, 2017

Member

Should not we colorize the output rather than Bundler:?

jekyll-new-output

@DirtyF

DirtyF Jan 6, 2017

Member

Should not we colorize the output rather than Bundler:?

jekyll-new-output

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 18, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Jan 18, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 477b2f0 into master Jan 18, 2017

2 checks passed

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

@jekyllbot jekyllbot deleted the jekyll-utils-exec branch Jan 18, 2017

jekyllbot added a commit that referenced this pull request Jan 18, 2017

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