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

Allows creation of site in current directory #549

Merged
merged 2 commits into from May 14, 2015
Merged

Allows creation of site in current directory #549

merged 2 commits into from May 14, 2015

Conversation

@TheLonelyGhost
Copy link
Contributor

@TheLonelyGhost TheLonelyGhost commented May 1, 2015

Had issues when moving from Jekyll to nanoc with Github Pages where it didn't properly detect if there was a site that existed already.

I'm open to suggestions where detecting the presence of 'nanoc.yaml' assumes a site exists.

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 3, 2015

Could you elaborate on the issue you had where nanoc didn’t properly detect a site already existed? It should error out if the given destination directory already exists (even if it’s a file instead of a directory).

I’m uncertain whether this enhancement is useful, for two reasons:

  • The current behavior seems clear to me, even if it doesn’t match the behavior of git. There’s not much additional value to defaulting to the current directory.
  • The current behavior does not match the behavior of git on purpose: creating a site in the current directory might create a lot of new files, potentially overwriting some, and undoing that is a pain.

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 4, 2015

You're right that defaulting to the current directory is less clear than the current documentation.

Circumstances:

  • I have a project hosted locally in ~/myproject containing a super awesome Ruby gem that I am now circling back to update documentation.
  • Project docs are being kept in a separate branch of the repository, gh-pages, which only contains Rust project source code necessary for documentation.
  • Current docs use Jekyll to render them as a small website using Github Pages, but I want to use nanoc instead of Jekyll.
  • I create a fresh branch, docs, for the nanoc which will contain the nanoc source files.
  • I will output the generated assets for the nanoc site to the gh-pages branch, completely replacing the Jekyll source files.
  • The current working directory is ~/myproject and the currently checked-out branch is doc.

Given the above, I have 3 options:

  1. cd into ~/myproject and run nanoc create-site
    • Gives command usage instead of creating in current directory
  2. cd into ~/myproject and run nanoc create-site ./
    • Fails saying a site already exists instead of creating in current directory
  3. cd into ~/ and run nanoc create-site myproject
    • Fails saying a site already exists instead of creating in specified directory

With this pull request, all three scenarios would react as expected.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 11, 2015

Being able to create a site into an empty directory (which fits your use case) seems useful. I’d change the if File.exist?(File.join(path, 'nanoc.yaml')) check (formerly if File.exist?(path)) to be a little safer, though. I recommend checking whether either the file does not exist at all, or is an empty directory:

if File.exist?(path) && (!File.directory?(path) || Dir.entries(path) != ['.', '..'])
  # …
end

I prefer to have the second argument be mandatory. Having a default argument is confusing.

Also, I’m afraid I’ve done some work on the create-site command and your changes are not longer mergeable. Could you rebase onto master?

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 11, 2015

Rebasing now. I'm removing the sections where it defaults to the current directory since you make valid points.

While working on the section about empty directory, I ran into an issue. Checking if the directory is empty may not be right response either since my use case assumes a fresh git branch. With git, even a fresh branch will not be empty since it must contain the .git directory. I hardly think hardcoding exceptions for each version control system would be a good option either.

Would a prompt requiring confirmation be better if the directory is not empty?

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 11, 2015

If you mean an interactive prompt, then I’d say “no”, simply because there are no interactive parts in the nanoc CLI elsewhere.

Printing a warning and aborting when the given directory exists and is not empty makes sense. I’d say that message could say this:

The given directory, (full path here), exists and is not empty. If this is truly where you want the nanoc site to be created, re-run this command with --force.

Also, a new --force that overrides the non-empty-directory check.

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 13, 2015

Now that master has stable builds again, looks like tests will pass. 👍

I've limited the current directory functionality PR to this one and split off the --force option discussed above to #580. I'll let you choose if one or both are merged in.

Loading

@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 14, 2015

👍

Loading

ddfreyne added a commit that referenced this issue May 14, 2015
…me-directory

Allows creation of site in current directory
@ddfreyne ddfreyne merged commit c4278c2 into nanoc:master May 14, 2015
1 check passed
Loading
@ddfreyne
Copy link
Member

@ddfreyne ddfreyne commented May 14, 2015

Build on master is failing but that’s unrelated to this PR (and I can’t even reproduce it locally…)

Loading

@TheLonelyGhost
Copy link
Contributor Author

@TheLonelyGhost TheLonelyGhost commented May 14, 2015

Seems that Travis-CI has had issues in the past week so it might be related to that. Either way, the final commit that was merged had a successful build so it truly is unrelated to this PR.

Loading

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