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

Refactor `Site#cleanup` #1429

Merged
merged 6 commits into from Aug 26, 2013

Conversation

Projects
None yet
4 participants
@maul-esel
Contributor

maul-esel commented Aug 17, 2013

Move the cleanup logic out into a separate class, Jekyll::Cleaner.

This heavily reduces the complexity of the Site class. Behaviour should all stay the same, thus I added no new tests.

Refactor `Site#cleanup`
Move the cleanup logic out into a separate class,
`Jekyll::Cleaner`.
@@ -89,6 +87,8 @@ def setup
self.converters = instantiate_subclasses(Jekyll::Converter)
self.generators = instantiate_subclasses(Jekyll::Generator)
@site_cleaner = Cleaner.new(self)

This comment has been minimized.

@parkr

parkr Aug 17, 2013

Member

Would you mind using an attr_accessor for this instead? The rest of the Site class uses this convention and I'd prefer to stick with that.

This comment has been minimized.

@maul-esel

maul-esel Aug 17, 2013

Contributor

✔️

@parkr

This comment has been minimized.

Member

parkr commented Aug 17, 2013

Cool! Always happy to see refactoring for simplicity's sake.

@@ -0,0 +1,69 @@
require 'set'
module Jekyll

This comment has been minimized.

@mattr-

mattr- Aug 18, 2013

Member

Might be a good idea to put this inside of a Site module.

This comment has been minimized.

@maul-esel

maul-esel Aug 18, 2013

Contributor

Like this:

module Jekyll
  module Site
    class Cleaner
      ...
    end
  end
end

This comment has been minimized.

@mattr-

mattr- Aug 18, 2013

Member

Yup 😃

This comment has been minimized.

@maul-esel

maul-esel Aug 19, 2013

Contributor

jekyll/lib/jekyll/cleaner.rb:4:in `module:Jekyll': Site is not a module (TypeError)

This comment has been minimized.

@maul-esel

maul-esel Aug 19, 2013

Contributor

So what about

module Jekyll
  class Site
    class Cleaner
      ...
    end
  end
end

That seems to work.

This comment has been minimized.

@mattr-

mattr- Aug 19, 2013

Member

Fine with me. 😃

On Mon, Aug 19, 2013 at 1:27 PM, maul-esel notifications@github.com wrote:

In lib/jekyll/cleaner.rb:

@@ -0,0 +1,69 @@
+require 'set'
+
+module Jekyll

So what about

module Jekyll
class Site

class Cleaner
  ...
end

endend

[image: ] That seems to work.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1429/files#r5849989
.

# Cleans up the site's destination directory
def cleanup!
obsolete_files = existing_files - new_files - new_dirs + replaced_files

This comment has been minimized.

@mattr-

mattr- Aug 18, 2013

Member

obsolete_files should be a method that does this calculation and returns an array.

def existing_files
files = Set.new
Dir.glob(File.join(@site.dest, "**", "*"), File::FNM_DOTMATCH) do |file|
if @site.keep_files.length > 0

This comment has been minimized.

@mattr-

mattr- Aug 18, 2013

Member

this conditional could be it's own method.

This comment has been minimized.

@maul-esel

maul-esel Aug 19, 2013

Contributor

Actually, I've often thought that this conditional is unnecessary: Always match for the regex, and if it's empty, it works anyway. To test this, I reduced the code to:

Dir.glob(File.join(@site.dest, "**", "*"), File::FNM_DOTMATCH) do |file|
  files << file unless file =~ /\/\.{1,2}$/ || file =~ keep_file_regex
end

All tests still pass. So unless this is due to a missing test, I'll remove the condition.

This comment has been minimized.

@maul-esel

This comment has been minimized.

@parkr

parkr Aug 22, 2013

Member

The reason that conditional existed was in order to avoid a Nil error when comparing file to nil.

This comment has been minimized.

@maul-esel

maul-esel Aug 22, 2013

Contributor

But as keep_file_regex never returns nil, it should no longer be necessary?

This comment has been minimized.

@parkr

parkr Aug 23, 2013

Member

Precisely!

module Jekyll
class Site
attr_accessor :config, :layouts, :posts, :pages, :static_files,
:categories, :exclude, :include, :source, :dest, :lsi, :pygments,
:permalink_style, :tags, :time, :future, :safe, :plugins, :limit_posts,
:show_drafts, :keep_files, :baseurl
:show_drafts, :keep_files, :baseurl, :site_cleaner

This comment has been minimized.

@mattr-

mattr- Aug 18, 2013

Member

i don't think we need an attr_accessor for this. it's an implementation detail of Site and it doesn't make sense to expose it.

This comment has been minimized.

@parkr

parkr Aug 19, 2013

Member

It follows convention. We ought not use @site_cleaner. I'd be OK with

def site_cleaner
  @site_cleaner ||= Jekyll::Site::Cleaner.new(self)
end

as an alternative.

This comment has been minimized.

@mattr-

mattr- Aug 19, 2013

Member

convention schmonvention in this case. 😃

I like the memoization idea and just to be clear, I would make that method
private.

On Mon, Aug 19, 2013 at 11:32 AM, Parker Moore notifications@github.comwrote:

In lib/jekyll/site.rb:

module Jekyll
class Site
attr_accessor :config, :layouts, :posts, :pages, :static_files,
:categories, :exclude, :include, :source, :dest, :lsi, :pygments,
:permalink_style, :tags, :time, :future, :safe, :plugins, :limit_posts,

  •              :show_drafts, :keep_files, :baseurl
    
  •              :show_drafts, :keep_files, :baseurl, :site_cleaner
    

It follows convention. We ought not use @site_cleaner. I'd be OK with

def site_cleaner
@site_cleaner ||= Jekyll::Site::Cleaner.new(self)end

as an alternative.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1429/files#r5846781
.

This comment has been minimized.

@parkr

parkr Aug 20, 2013

Member

I'd really prefer to stick with convention always 😄

@maul-esel Thanks for implementing it! I'm down with making it a private method.

@parkr

This comment has been minimized.

parkr commented on lib/jekyll/site.rb in d28858a Aug 20, 2013

Would you mind making this method private, please? :)

This comment has been minimized.

Owner

maul-esel replied Aug 21, 2013

It is private, see line 377.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Aug 25, 2013

Adressed all the comments, anything left to do?

@mattr-

This comment has been minimized.

Member

mattr- commented Aug 26, 2013

LGTM. @parkr?

parkr added a commit that referenced this pull request Aug 26, 2013

Merge pull request #1429 from maul-esel/refactor-cleanup
Refactor `Site#cleanup` into Jekyll::Site::Cleaner class

@parkr parkr merged commit 8ea3369 into jekyll:master Aug 26, 2013

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Aug 26, 2013

@maul-esel maul-esel deleted the maul-esel:refactor-cleanup branch Aug 27, 2013

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