Keep files feature #685

Merged
merged 5 commits into from Jan 11, 2013

4 participants

@edeustace

As requested - a version of pull request #630 except its now on a feature branch on the fork.

@parkr
Jekyll member

Nicely alphabetized.

@parkr
Jekyll member

Maybe we should add '.svn'?

Jekyll member

I'd say 'yes', at least for the corporate environments (like mine) where svn is the standard and the use of git is nonexistant (sadly).

@parkr
Jekyll member

Thoughts on using or_list = self.keep_files.join("|")?

In this case, it produces the exact same output.

Jekyll member

👍 to this. More understandable that way too. The current line just makes my head hurt.

@parkr
Jekyll member

Lastly, are these from the latest master?

@edeustace

Hi @parkr, I just rebased from mojombo/jekyll but was already at latest. I've made those changes you suggested.

@parkr
Jekyll member

Great! +1 for this change, @mojombo!

@parkr
Jekyll member

And thanks, @edeustace :)

@parkr parkr commented on an outdated diff Dec 18, 2012
jekyll.gemspec
lib/jekyll/migrators/marley.rb
lib/jekyll/migrators/mephisto.rb
lib/jekyll/migrators/mt.rb
lib/jekyll/migrators/posterous.rb
+ lib/jekyll/migrators/rss.rb
@parkr
Jekyll member
parkr added a line comment Dec 18, 2012

Were these migrators forgotten?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mattr-
Jekyll member

I think I would rename this to something like:

should 'recursively keeps files as specified in the config' do

or something like that, just to make the test case description more - heh - descriptive, but I'm not really bothered by the current description either.

@mattr-
Jekyll member

Sorry, I'm late to the commenting party for this pull. You can ignore my comment about the use of .join since it was already done.

@mojombo

Can you rebase this without the gemspec changes? It won't merge cleanly with those in there.

@mojombo mojombo commented on an outdated diff Jan 1, 2013
lib/jekyll/site.rb
@@ -217,7 +219,11 @@ def cleanup
# all files and directories in destination, including hidden ones
dest_files = Set.new
Dir.glob(File.join(self.dest, "**", "*"), File::FNM_DOTMATCH) do |file|
- dest_files << file unless file =~ /\/\.{1,2}$/
+ if self.keep_files.length > 0
+ dest_files << file unless file =~ /\/\.{1,2}$/ or file =~ keep_file_regex
@mojombo
mojombo added a line comment Jan 1, 2013

I'd prefer to see || here instead of or.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojombo mojombo commented on an outdated diff Jan 1, 2013
lib/jekyll/site.rb
@@ -242,6 +248,14 @@ def cleanup
FileUtils.rm_rf(obsolete_files.to_a)
end
+ # create a regex from the keep_files array
+ # ['.git','.svn'] => /\/(\.git|\/.svn)/
@mojombo
mojombo added a line comment Jan 1, 2013

I'm trying to convert all the docs to TomDoc. I'd love to see these done in that style.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mojombo mojombo commented on the diff Jan 1, 2013
test/test_site.rb
@@ -195,6 +205,23 @@ class TestSite < Test::Unit::TestCase
assert !File.exist?(dest_dir('obsolete.html'))
assert !File.exist?(dest_dir('qux'))
assert !File.exist?(dest_dir('quux'))
+ assert File.exist?(dest_dir('.git'))
+ assert File.exist?(dest_dir('.git/HEAD'))
+ end
+
+ should 'remove orphaned files in destination - keep_files .svn' do
+
@mojombo
mojombo added a line comment Jan 1, 2013

Extraneous whitespace.

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

Been away, will have a look later.

@parkr
Jekyll member

@edeustace You may need to rebase on the current master again, sorry!

@edeustace

@parkr hows this commit?

@parkr parkr merged commit 418ef41 into jekyll:master Jan 11, 2013

1 check passed

Details default The Travis build passed
@parkr
Jekyll member

It's great, thanks!

@edeustace edeustace deleted the edeustace:keep_files_feature branch Jan 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment