Skip to content
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

Adds --force option to create site in non-empty directory #580

Merged
merged 2 commits into from May 15, 2015
Merged

Adds --force option to create site in non-empty directory #580

merged 2 commits into from May 15, 2015

Conversation

@TheLonelyGhost
Copy link
Contributor

@TheLonelyGhost TheLonelyGhost commented May 13, 2015

Continuation of #549. Allows --force flag to override the site-already-exists safeguard

@@ -281,7 +282,7 @@ def run

# Check whether site exists
if File.exist?(path)
Copy link
Member

@ddfreyne ddfreyne May 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the unless options[:force] next to the condition here?

if File.exist?(path) && !options[:force]

The options[:force] will otherwise be overlooked too easily, I feel.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 14, 2015

Looks nice so far!

For an even better user experience, you could check whether path is a directory, and if it is, allow a site to be created with --force; it if is not a directory, don’t allow the site to be created (it would fail anyway).

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 14, 2015

You’ll have to rebase on top of master; the other PR conflicts with this one.

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 14, 2015

It was originally based off of the other PR, but I figured it would be better kept as independent features in case one was accepted and the other rejected.

Rebased from master and moved the !options[:force] condition as per your suggestion while addressing merge conflicts.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 15, 2015

All good except for the message itself. “A site already exists” is incorrect. I propose the following:

The site was not created, because #{path} already exists. If you really want to create a site in this directory, re-run this command with --force.

What do you think?

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 15, 2015

I feel like your suggestion is a bit wordy, but I can't think of an alternative that gives the same feel as the other messages in a more concise manner. Copypasta verbatim into the new error message.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 15, 2015

Thanks!

Loading

ddfreyne added a commit that referenced this issue May 15, 2015
…nempty-directory

Adds --force option to create site in non-empty directory
@ddfreyne ddfreyne merged commit 67df5ce into nanoc:master May 15, 2015
1 check passed
Loading
@TheLonelyGhost TheLonelyGhost deleted the feature/create-site-in-nonempty-directory branch May 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants