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

Add --force flag to new command #1115

Merged
merged 5 commits into from May 18, 2013

Conversation

Projects
None yet
4 participants
@caseylang
Contributor

caseylang commented May 15, 2013

As suggested in #1104, I've added a --force command to the new subcommand.

@caseylang

This comment has been minimized.

Show comment
Hide comment
@caseylang

caseylang May 15, 2013

Contributor

I've noticed if foo && bar == false being used instead of if foo && !bar in a few places. If you prefer that, please let me know and I'll be happy to change.

Contributor

caseylang commented May 15, 2013

I've noticed if foo && bar == false being used instead of if foo && !bar in a few places. If you prefer that, please let me know and I'll be happy to change.

@@ -3,12 +3,12 @@
module Jekyll
module Commands
class New < Command
def self.process(args)
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)

This comment has been minimized.

@parkr

parkr May 15, 2013

Member

In your tests, did args include --force?

@parkr

parkr May 15, 2013

Member

In your tests, did args include --force?

This comment has been minimized.

@caseylang

caseylang May 16, 2013

Contributor

No, flags were properly separated from args. If you ran the command with only the force flag args would simply be an empty array. Commander is nice enough to ensure this in their specs.

@caseylang

caseylang May 16, 2013

Contributor

No, flags were properly separated from args. If you ran the command with only the force flag args would simply be an empty array. Commander is nice enough to ensure this in their specs.

This comment has been minimized.

@parkr

parkr May 16, 2013

Member

Awesome, thanks for confirming.

@parkr

parkr May 16, 2013

Member

Awesome, thanks for confirming.

Show outdated Hide outdated lib/jekyll/commands/new.rb
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
unless Dir["#{new_blog_path}/**/*"].empty?
if !options[:force] && !Dir["#{new_blog_path}/**/*"].empty?

This comment has been minimized.

@mattr-

mattr- May 16, 2013

Member

I think I'd like to see something like this here instead:

if preserve_source_location?(new_blog_path, options)

where preserve_source_location? is something like so:

def self.preserve_source_location?(new_blog_path, options)
   !options[:force] && !Dir["#{new_blog_path}/**/*"].empty?
end

It's an additional abstraction but one that provides more clarity.

@mattr-

mattr- May 16, 2013

Member

I think I'd like to see something like this here instead:

if preserve_source_location?(new_blog_path, options)

where preserve_source_location? is something like so:

def self.preserve_source_location?(new_blog_path, options)
   !options[:force] && !Dir["#{new_blog_path}/**/*"].empty?
end

It's an additional abstraction but one that provides more clarity.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 17, 2013

Member

This looks good to me. @mattr-, if everything checks out for you as well, merge it in! :)

Member

parkr commented May 17, 2013

This looks good to me. @mattr-, if everything checks out for you as well, merge it in! :)

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- May 18, 2013

Member

Nice work @clang1234 ! Thanks! ❤️ 🚢ed

Member

mattr- commented May 18, 2013

Nice work @clang1234 ! Thanks! ❤️ 🚢ed

mattr- added a commit that referenced this pull request May 18, 2013

Merge pull request #1115 from clang1234/master
Add --force flag to new command

@mattr- mattr- merged commit 7c012a9 into jekyll:master May 18, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request May 18, 2013

@caseylang

This comment has been minimized.

Show comment
Hide comment
@caseylang

caseylang May 18, 2013

Contributor

Thanks guys! Glad I could help.

Contributor

caseylang commented May 18, 2013

Thanks guys! Glad I could help.

@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.