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

Cleaner empty dirs #2458

Merged
merged 3 commits into from May 30, 2014

Conversation

Projects
None yet
4 participants
@karouf
Contributor

karouf commented May 27, 2014

This slightly changes the way Cleaner handles directories containing no files but non-empty directories:

  • if there's a new file to be created it doesn't only keep the parent directory but all the parent directories up to the destination directory
  • if there's a directory in keep_files, it removes it before clean-up from the existing files as well as all its parent directories up to the destination directory
@@ -49,7 +49,19 @@ def new_files
#
# Returns a Set with the directory paths
def new_dirs
new_files.map { |file| File.dirname(file) }.to_set
new_files.map { |file| parent_dirs(file) }.flatten.to_set

This comment has been minimized.

@parkr

parkr May 27, 2014

Member

Do you think #to_set or #uniq is a better option here?

This comment has been minimized.

@karouf

karouf May 28, 2014

Contributor

Do you mean instead of #flatten? I don't really like to have it here but I also felt that #parent_dirs shouldn't have to deal with that either to keep it simple.

This comment has been minimized.

@parkr

parkr May 28, 2014

Member

No i mean instead of #to_set! Wouldn't #uniq achieve the same end?

This comment has been minimized.

@karouf

karouf May 28, 2014

Contributor

Well #parent_dirs returns an Array so calling #to_set on new_files.map{} does both filter out duplicate directories and return a Set as is expected from #new_dirs.
Using #uniq would also work though but #new_dirs would return an Array.

if parent_dir == site.dest
return []
else
return [parent_dir] + parent_dirs(parent_dir)

This comment has been minimized.

@parkr

parkr May 29, 2014

Member

Please remove the explicit return statements here. (It's implicit)

@parkr

This comment has been minimized.

Member

parkr commented May 29, 2014

Other than the quick code style comment above, this LGTM! @mattr-, you wrote the Cleaner I think. Care to lend your 👀 for a sec?

@mattr-

This comment has been minimized.

Member

mattr- commented May 30, 2014

👍 from me, other than the existing comments about style.

@parkr parkr referenced this pull request May 30, 2014

Closed

Release Jekyll 2.1.0 #2465

5 of 7 tasks complete
@karouf

This comment has been minimized.

Contributor

karouf commented May 30, 2014

I just updated the PR according to your comments.

@parkr

This comment has been minimized.

Member

parkr commented May 30, 2014

LGTM, thank you!!

parkr added a commit that referenced this pull request May 30, 2014

@parkr parkr merged commit e9ac432 into jekyll:master May 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

parkr added a commit that referenced this pull request May 30, 2014

@karouf karouf deleted the karouf:cleaner-empty-dirs branch Nov 23, 2015

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