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

Added a --blank command which scaffolds empty files. #1171

Merged
merged 10 commits into from Jun 25, 2013

Conversation

Projects
None yet
5 participants
@zachgersh
Contributor

zachgersh commented Jun 2, 2013

Fixes #1134

Welcome any feedback on the approach that was taken.

@parkr

This comment has been minimized.

Member

parkr commented Jun 2, 2013

Thanks for taking care of this! I was thinking actualy of deleting all files we generate and instead creating the following:

./_layouts
./_posts
index.html # 0 bytes

@mattr-, what do you think?

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 2, 2013

I thought about that as well but figured it would be more descriptive to a user to just leave all the posts there so they have the more complete skeleton. Your suggestion is super easy.

Also, I need to add a test for blank! Completely forgot that.

@mattr-

This comment has been minimized.

Member

mattr- commented Jun 3, 2013

I like the idea. I think this will need cucumber tests though rather than unit tests.

I like the idea of removing all the posts and keeping just the _layouts and _posts directories and an empty index.html file.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 3, 2013

Cool will definitely make those changes and add the tests.

On Jun 2, 2013, at 8:59 PM, Matt Rogers notifications@github.com wrote:

I like the idea. I think this will need cucumber tests though rather than unit tests.

I like the idea of removing all the posts and keeping just the _layouts and _posts directories and an empty index.html file.


Reply to this email directly or view it on GitHub.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 4, 2013

Hey @parkr or @mattr-, needed a small amount of your wisdom.

I am currently trying to figure out via Cucumber if the _layouts / _posts directories exists after running the new --blank command. I think I am executing the first part correctly but I am actually unsure if anything is getting written to test directory.

Could you glance at my test and point out how horribly wrong it is? Thanks!

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 4, 2013

Never you mind :) I fixed my tests though I would love for a spot check!

@mattr-

View changes

features/step_definitions/jekyll_steps.rb Outdated
Dir.chdir(File.expand_path("..", TEST_DIR))
FileUtils.rm_rf(TEST_DIR)
end
#After do

This comment has been minimized.

@mattr-

mattr- Jun 4, 2013

Member

If this code isn't necessary anymore, can we remove this code instead of commenting it out?

This comment has been minimized.

@zachgersh

zachgersh Jun 4, 2013

Contributor

That's actually a great catch Matt! That code need to be there. I commented it out so I could look at the directory that was getting creates and make sure it was correct :)

Let me add that back.

On Jun 4, 2013, at 6:23 AM, Matt Rogers notifications@github.com wrote:

In features/step_definitions/jekyll_steps.rb:

@@ -4,15 +4,19 @@
Dir.chdir(TEST_DIR)
end

-After do

  • Dir.chdir(File.expand_path("..", TEST_DIR))
  • FileUtils.rm_rf(TEST_DIR)
    -end
    +#After do
    If this code isn't necessary anymore, can we remove this code instead of commenting it out?


Reply to this email directly or view it on GitHub.

@mattr-

This comment has been minimized.

Member

mattr- commented Jun 4, 2013

👍 from me.

@parkr

This comment has been minimized.

Member

parkr commented Jun 4, 2013

I have an implementation question. Why not just prevent the creation of the files in the first place if options[:blank] == true? Why delete them instead? It seems way less efficient to create a bunch of stuff and go back and delete it than just not create it in the first place.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 4, 2013

Hey @parkr,

I struggled with that thought as well. I figured it better to reuse the same function we are already using to generate the sample site and just remove the files since I keep all of the removal self contained.

To be more in line with your suggestion I could just create the two directories and then touch on index.html and that probably would be more efficient but do we really gain that much?

Glad to switch it based on your preference 😄

@parkr

This comment has been minimized.

Member

parkr commented Jun 4, 2013

What we also gain, and something I neglected to mention, is clarity. The current implementation requires we know exactly which directories and files are created. What if we added a js/ directory to automatically bundle jQuery or something? We'd have to update both sections instead of just the one. As a side note, the logic for this should be in a separate method.

def self.process(args, options = {})
  raise ArgumentError.new('You must specify a path.') if args.empty?

  new_blog_path = File.expand_path(args.join(" "), Dir.pwd)
  FileUtils.mkdir_p new_blog_path
  if preserve_source_location?(new_blog_path, options)
    Jekyll.logger.error "Conflict:", "#{new_blog_path} exists and is not empty."
    exit(1)
  end

  if options[:blank]
    create_blank_scaffold new_blog_path
  else
    create_sample_files new_blog_path
  end

  File.open(File.expand_path(self.initialized_post_name, new_blog_path), "w") do |f|
    f.write(self.scaffold_post_content(site_template))
  end

  puts "New jekyll site installed in #{new_blog_path}."
end

def self.create_blank_scaffold(path)
  FileUtils.mkdir_p(path)
  Dir.chdir(path) do
    FileUtils.mkdir(%w[_layouts _posts])
    FileUtils.touch("index.html")
  end
end

They seem like two separate concerns - one creates a scaffold of a site and one creates a site, v1.0 before any real content is added (but it's still publishable).

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 4, 2013

Agree 100% with you about the separate method and the way forward. I will refactor this tonight and it will definitely be more clear / maintainable.

Great suggestion.

On Jun 4, 2013, at 9:45 AM, Parker Moore notifications@github.com wrote:

What we also gain, and something I neglected to mention, is clarity. The current implementation requires we know exactly which directories and files are created. What if we added a js/ directory to automatically bundle jQuery or something? We'd have to update both sections instead of just the one. As a side note, the logic for this should be in a separate method.

def self.process(args, options = {})
raise ArgumentError.new('You must specify a path.') if args.empty?

new_blog_path = File.expand_path(args.join(" "), Dir.pwd)
FileUtils.mkdir_p new_blog_path
if preserve_source_location?(new_blog_path, options)
Jekyll.logger.error "Conflict:", "#{new_blog_path} exists and is not empty."
exit(1)
end

if options[:blank]
create_blank_scaffold new_blog_path
else
create_sample_files new_blog_path
end

File.open(File.expand_path(self.initialized_post_name, new_blog_path), "w") do |f|
f.write(self.scaffold_post_content(site_template))
end

puts "New jekyll site installed in #{new_blog_path}."
end

def self.create_blank_scaffold(path)
FileUtils.mkdir_p(path)
Dir.chdir(path) do
FileUtils.mkdir(%w[_layouts _posts])
FileUtils.touch("index.html")
end
end
They seem like two separate concerns - one creates a scaffold of a site and one creates a site, v1.0 before any real content is added (but it's still publishable).


Reply to this email directly or view it on GitHub.

@parkr

This comment has been minimized.

Member

parkr commented Jun 4, 2013

Thanks mate!

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 5, 2013

You have to love when the travis build fails due to a time issue ^

@parkr

View changes

lib/jekyll/commands/new.rb Outdated
create_blank_site new_blog_path
else
create_sample_files new_blog_path
end
File.open(File.expand_path(self.initialized_post_name, new_blog_path), "w") do |f|
f.write(self.scaffold_post_content(site_template))
end

This comment has been minimized.

@parkr

parkr Jun 5, 2013

Member

This block should be skipped for the blank site, right?

@parkr

View changes

lib/jekyll/commands/new.rb Outdated
puts "New jekyll site installed in #{new_blog_path}."
end
def self.create_blank_site(path)
Dir.chdir(path) do
FileUtils.mkdir(%w(_layouts _posts))

This comment has been minimized.

@parkr

parkr Jun 5, 2013

Member

I think we should also add _drafts here.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 5, 2013

Done and Done @parkr great catch on that needing to be in the else block.

I am going to refactor my test a little bit to make sure the directory is empty but this should be fine to merge once Travis is done (unless you have more changes).

@parkr

This comment has been minimized.

Member

parkr commented Jun 5, 2013

Great! Thanks for your work on this @zachgersh :)

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 5, 2013

Hey @maul-esel I like the suggestion. I am actually going to refactor the tests to make them a bit better and can take that into account as well.

I am assuming you are suggestion removing the check for the test_blank path and just working that into each file test?

@parkr were you going to merge this today or wait for the refactor? I am fine with either.

@mattr-

This comment has been minimized.

Member

mattr- commented Jun 5, 2013

I say let's wait. There's no reason to rush to merge this in

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 5, 2013

Sounds good @mattr- I am going to refactor those tests and then we should be good.

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 10, 2013

I actually review this and I am happy with the code (no refactor needed). Responded to @maul-esel comment inline.

Please merge when ready :)

@mattr-

View changes

features/step_definitions/jekyll_steps.rb Outdated
Then /^the (.*) directory should exist in the (.*) path$/ do |dir, path|
assert File.directory?("#{TEST_DIR}/#{path}/#{dir}"),
"The directory \"#{dir}\" does not exist"

This comment has been minimized.

@mattr-

mattr- Jun 10, 2013

Member

Can you indent this so that it inlines up with the other parameter? I originally thought it was a separate line of code.

This comment has been minimized.

@mattr-

mattr- Jun 10, 2013

Member
assert File.directory?("#{TEST_DIR}/#{path}/#{dir}"),
       "The directory \"#{dir}\" does not exist"

is what I'm after here.

Thanks! ❤️

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 10, 2013

thanks again @maul-esel, I think these were the changes you were suggesting!

@@ -14,6 +14,15 @@ def run_jekyll(opts = {})
system command
end
def call_jekyll_new(opts = {})

This comment has been minimized.

@parkr

parkr Jun 10, 2013

Member

Maybe we can create a generic method to which the step definition passes the command name?

This comment has been minimized.

@zachgersh

zachgersh Jun 11, 2013

Contributor

I could go with that but I was also thinking maybe I just explicitly name the method scaffold_blank. Makes the method definition more explicit?

This comment has been minimized.

@zachgersh

zachgersh Jun 18, 2013

Contributor

What was your opinion here?

def self.create_blank_site(path)
Dir.chdir(path) do
FileUtils.mkdir(%w(_layouts _posts _drafts))
FileUtils.touch("index.html")

This comment has been minimized.

@parkr

parkr Jun 10, 2013

Member

Hm... just thinking. Should this contain the top YAML header?

This comment has been minimized.

@zachgersh

zachgersh Jun 11, 2013

Contributor

Up to you! I think it's fine blank since it is supposed to be totally blank anyway!

This comment has been minimized.

@zachgersh

zachgersh Jun 18, 2013

Contributor

@parkr I was fine with blank but if you want this I can add it.

@@ -3,6 +3,13 @@ Feature: Create sites
I want to be able to make a static site
In order to share my awesome ideas with the interwebs
Scenario: Blank site
Given I do not have a "test_blank" directory

This comment has been minimized.

@maul-esel

maul-esel Jun 10, 2013

Contributor

Is this really necessary?

This comment has been minimized.

@zachgersh

zachgersh Jun 11, 2013

Contributor

I assume you are referring to the Given? All other features test for the directory, I wanted to make sure we started with no directory.

Are you suggestion I change it to "Given I have the "test_blank" directory? The current test for directory actually creates the directory. I don't see too much of an issue with this one.

This comment has been minimized.

@maul-esel

maul-esel Jun 11, 2013

Contributor

First one - a lot of things do depend on an initially clean dir, don't they? No big problem though, was just wondering...

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jun 10, 2013

@zachgersh: That's what I meant. Also see the other comment inline.

@parkr

This comment has been minimized.

Member

parkr commented Jun 18, 2013

@mattr- This looks good to me!

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 18, 2013

I just rebased this to make sure it was ready to merge into master (if @mattr- is cool with it).

@zachgersh

This comment has been minimized.

Contributor

zachgersh commented Jun 24, 2013

@mattr-, since you are merging all sorts of stuff today :)

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

Merge pull request #1171 from zachgersh/new_empty
Added a --blank command which scaffolds empty files.

@mattr- mattr- merged commit fdebe49 into jekyll:master Jun 25, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jun 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.