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

Refactored site.rb, #1144

Merged
merged 2 commits into from Jul 12, 2013

Conversation

Projects
None yet
5 participants
@jpiasetz
Copy link
Contributor

jpiasetz commented May 24, 2013

I tried to clean a bunch of the methods code climate was complaining about.

@kelvinst

This comment has been minimized.

Copy link

kelvinst commented Jul 5, 2013

There are some refactorings that I'd liked:

  • Extracted the Site#each method
  • Had followed come ruby good code conventions

It was a very nice pull request... But I had some suggestions (line notes)

@kelvinst

View changes

lib/jekyll/site.rb Outdated
@@ -430,5 +401,24 @@ def relative_permalinks_deprecation_method
@deprecated_relative_permalinks = true
end
end

def each

This comment has been minimized.

Copy link
@kelvinst

kelvinst Jul 5, 2013

The method intent is good, but I suggest to change the method name cause this method can be accessed from outside the class and it will seems weird:
e.g.

site = Site.new
site.each do |item|
  # code omitted
end

each what? What kind of item a site have? Seems a little confused, can be many different things

I suggest to call it each_file

This comment has been minimized.

Copy link
@parkr

parkr Jul 5, 2013

Member

I agree that it should be more specific – at first glance it almost seems like it should be Site.each which iterates over Site objects or something. Site#each_file is more descriptive.

This comment has been minimized.

Copy link
@jpiasetz

jpiasetz Jul 5, 2013

Author Contributor

I'll change the name however I think it is a symptom of another problem. post.rb and page.rb have high duplication. I've got another branch that I'm working on extracting a parent from the two of them. I think if I can work that out then this each should move into that.

@kelvinst

View changes

lib/jekyll/site.rb Outdated
first3 = File.open(f_abs) { |fd| fd.read(3) }
if first3 == "---"
# file appears to have a YAML header so process it as a page
if !File.directory?(f_abs)

This comment has been minimized.

Copy link
@kelvinst

kelvinst Jul 5, 2013

Two little code smells:

  • Prefer use unless something instead of if !something
# bad
if !success
  puts "fail!"
end

# good
unless success
  puts "fail!"
end
  • And if you only change to unless, that it will have another code smell: never use unless with else. Rewrite these with the positive case first.
# bad
unless success
  puts "failure"
else
  puts "success"
end

# good
if success
  puts "success"
else
  puts "failure"
end

My suggestion is chage this piece of code to:

if File.directopry?(f_abs)
  f_rel = File.join(dir, f)
  read_directories(f_rel) unless self.dest.sub(/\/$/, '') == f_abs
else
  if is_yaml(f_abs)
    pages << Page.new(self, self.source, dir, f)
  else
    static_files << StaticFile.new(self, self.source, dir, f)
  end
end

If you have some time, take a read at GitHub Ruby Styleguide and bbatsov/ruby-style-guide

@kelvinst

View changes

lib/jekyll/site.rb Outdated

private

def is_yaml?(file)

This comment has been minimized.

Copy link
@kelvinst

kelvinst Jul 5, 2013

Just an opinion: Maybe the name of this method is not good enough, seems that the method does not really checks if it's a YAML file (has .yml extension), but check that the file starts with "---", that is the YAML header, I suggest call it has_yaml_header?

This comment has been minimized.

Copy link
@parkr

parkr Jul 5, 2013

Member

Agreed: has_yaml_header? makes more sense to me, too. Thanks for reviewing this, @kelvinst.

This comment has been minimized.

Copy link
@kelvinst

kelvinst Jul 5, 2013

I'm grateful to help

@kelvinst

This comment has been minimized.

Copy link

kelvinst commented Jul 5, 2013

Remembering, the notes are just fine adjustments... The refactoring are good enough to be on HEAD

@jpiasetz

This comment has been minimized.

Copy link
Contributor Author

jpiasetz commented Jul 5, 2013

@kelvinst thanks for all the help!

@kelvinst

This comment has been minimized.

Copy link

kelvinst commented Jul 5, 2013

You're welcome

@jpiasetz

This comment has been minimized.

Copy link
Contributor Author

jpiasetz commented Jul 9, 2013

@parkr are you ok with this as it stands? I've got some tangential changes that build off this. If it's close to being merged I'll hold off and create a new pull request otherwise I might try and rework this to include them.

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 9, 2013

Can you quickly run this branch through ruby-prof and link me to a gist with the output? I like the refactor so far. @mattr-.

Thanks!

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 9, 2013

Feel free to use this test site:

$ cd jekyll # wherever your jekyll fork is cloned to
$ git clone https://github.com/jekyll/test-site test-site && cd test-side
$ ruby-prof ../bin/jekyll build > ruby-prof-jekyll-site-refactor.txt
$ gist ruby-prof-jekyll-site-refactor.txt

It'd be great if you could use Ruby 1.9.3 to do so.

@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Jul 9, 2013

looks fine to me.

@jpiasetz

This comment has been minimized.

Copy link
Contributor Author

jpiasetz commented Jul 9, 2013

@parkr https://gist.github.com/anonymous/5959739 Anything in particularly you're looking for (I ran it in 1.9.3)?

@parkr

View changes

lib/jekyll/site.rb Outdated
self.static_files.each do |sf|
files << sf.destination(self.dest)
end
each_file { |item| files << item.destination(self.dest) }

This comment has been minimized.

Copy link
@parkr

parkr Jul 9, 2013

Member

each_site_file? each_file isn't very descriptive... @mattr-

This comment has been minimized.

Copy link
@parkr

parkr Jul 9, 2013

Member

site_files.each?

This comment has been minimized.

Copy link
@jpiasetz

jpiasetz Jul 9, 2013

Author Contributor

I think the end goal will end up with this method killed and end up with one array and a hierarchy like this:

files
|
jekyll_files static_files
|
posts pages
|
drafts

There is common functionality that should be hoisted out posts and pages. I think it makes sense for a site to contain files and as long as they quack right treat them all the same

This comment has been minimized.

Copy link
@mattr-

mattr- Jul 9, 2013

Member

We're in the Site object already though. each_file may not be very
descriptive, but it's an improvement over the each which was there
before, and each_site_file seems a bit redundant from my point of view.

On Tue, Jul 9, 2013 at 1:30 PM, Parker Moore notifications@github.comwrote:

In lib/jekyll/site.rb:

@@ -255,15 +240,7 @@ def cleanup

   # files to be written
   files = Set.new
  •  self.posts.each do |post|
    
  •    files << post.destination(self.dest)
    
  •  end
    
  •  self.pages.each do |page|
    
  •    files << page.destination(self.dest)
    
  •  end
    
  •  self.static_files.each do |sf|
    
  •    files << sf.destination(self.dest)
    
  •  end
    
  •  each_file { |item| files << item.destination(self.dest) }
    

each_site_file? each_file isn't very descriptive... @mattr-https://github.com/mattr-


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

This comment has been minimized.

Copy link
@mattr-

mattr- Jul 9, 2013

Member

site_files.each does seem a bit better.

Naming things is hard

On Tue, Jul 9, 2013 at 1:30 PM, Parker Moore notifications@github.comwrote:

In lib/jekyll/site.rb:

@@ -255,15 +240,7 @@ def cleanup

   # files to be written
   files = Set.new
  •  self.posts.each do |post|
    
  •    files << post.destination(self.dest)
    
  •  end
    
  •  self.pages.each do |page|
    
  •    files << page.destination(self.dest)
    
  •  end
    
  •  self.static_files.each do |sf|
    
  •    files << sf.destination(self.dest)
    
  •  end
    
  •  each_file { |item| files << item.destination(self.dest) }
    

site_files.each?


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

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Jul 9, 2013

One more comment (thought) then ready to merge.

@jpiasetz

This comment has been minimized.

Copy link
Contributor Author

jpiasetz commented Jul 11, 2013

@parkr did you have the dots in mind or the second _?

parkr added a commit that referenced this pull request Jul 12, 2013

Merge pull request #1144 from jpiasetz/cleanup
Refactor Jekyll::Site

@parkr parkr merged commit 979d5c1 into jekyll:master Jul 12, 2013

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jul 12, 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.
You can’t perform that action at this time.