Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Clean up Jekyll file types and site #1300

Closed
wants to merge 13 commits into from

5 participants

@jpiasetz

I tried to clean up the Pages, Posts, Layouts and StaticFiles and decouple them from site. @parkr I ran ruby-prof against it https://gist.github.com/5979489

@kelvinst you might want to take a look.

@mattr-
Owner

I appreciate your efforts to help clean up our codebase and I do like what you've done with several commits in this pull request. For example, 303bd56 makes that variable name much clearer and 2408c75 combines two operations into one, which is almost never a bad thing. :smiley:

I have a lot of questions about what you're trying to do with some of the changes you made though.

Why remove the LSI messages in 3113b61? They provide meaningful output for an operation that can take a long time.

Why raise exceptions in 6d098d7?

Why remove unnecessary self in the code (35086e7) when that's the style across all of the Jekyll code base?

Why combine Post, Page, and StaticFile? (0648d5f and e188911)

I know my various attempts at refactoring the codebase haven't gone well. A lot of this seems like you're just trying to remove the number of lines of code across the codebase vs. making targeted structural changes that improve design w/o affecting functionality. I looked through all the commits and they're all one line with no additional context to explain the change. Could you help me understand the thought process behind these changes?

@jpiasetz

@mattr- sorry don't mean to drop a big pull without explanation :(. I'll rebase them to have more explanatory commit comments. I have the bad habit of leading with code and then following with an explanation.

A lot of this seems like you're just trying to remove the number of lines of code across the codebase vs. making target structural changes that improve design w/o affecting functionality.

I wouldn't say the goal is line numbers but complexity. Site.rb is a monster that seems to be concerned with a lot of stuff and lacking a single responsibility. I think it's one responsibility should be managing the site files and a lot of the changes flow from that.

Why remove the LSI messages in 3113b61? They provide meaningful output for an operation that can take a long time

I wasn't sure why it was there. I guessed it was because it might take a long time. I was bold and took it out and figured if it was useful I would reopen the commit and edit the messages to say it would take a long time. It's also sort of surprising to have them write to the console. Especially if jekyll is include within something else.

Why raise exceptions in 6d098d7?

Tell Don't Ask - decisions about the state of the object should be made in the objects themselves. If Post is used outside of Site it should still tell you it can't be made if the name is wrong.

I'll go through and expand the other ones. Sorry again :( don't mean to come across as too much of a cowboy

@kelvinst

Sure... I will take a look @jpiasetz :smiley:

Thanks for the effort on reducing complexity (=

lib/jekyll/convertible.rb
@@ -26,18 +26,18 @@ def to_s
# name - The String filename of the file.
#
# Returns nothing.
- def read_yaml(base, name)
+ def read_yaml(path)

Good change, I always consider reducing param quantity a good thing... Since it continues making sense... As in this case...
Just only remember to update the TomDoc above (=

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/jekyll/site.rb
@@ -2,8 +2,8 @@
module Jekyll
class Site
- attr_accessor :config, :layouts, :posts, :pages, :static_files,
- :categories, :exclude, :include, :source, :dest, :lsi, :pygments,
+ attr_accessor :config, :layouts, :site_files,

I like this change... But, giving the context of the attributes, site_files seems a redundant name, since the attribute is a Site instance attribute... A practice of mine is ever think how the attribute looks like accessing it from out the class. For example:

site = Site.new
site.files

looks prettier than

site = Site.new
site.site_files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kelvinst kelvinst commented on the diff
lib/jekyll/site.rb
((67 lines not shown))
+ elsif path =~ /#{config['layouts']}/
+ ext = File.extname(path)
+ name = File.basename(path, ext)
+ layouts[name] = Layout.new(self, path)
+ nil
+ elsif path =~ /_posts/
+ Post
+ elsif path =~ /_drafts/ && show_drafts
+ Draft
+ elsif path.sub(source, '') =~ /\/_/
+ nil
+ elsif has_yaml_header?(path)
+ Page
+ else
+ StaticFile
+ end

I do not like these big chained if expressions
Maybe this can be better like this:

if File.file?(path) && path.sub(source, '') !~ /\/_/
  klass = if path =~ /_posts/
    Post
  elsif path =~ /_drafts/ && show_drafts
    Draft
  elsif has_yaml_header?(path)
    Page
  else
    StaticFile
  end
  files << klass.new(self, path)
elsif path =~ /#{config['layouts']}/
   ext = File.extname(path)
   name = File.basename(path, ext)
   layouts[name] = Layout.new(self, path)
end   

It will reduce a little the cyclomatic complexity of this statement (I'd manually calculated it, it became 2 paths less than the old one), and makes the code more simple to understand in my opinion...

And maybe this can be increased with a factory method for the first if statement

Yea I think this wants to be a factory. I just wasn't sure how to get it all the way. This is a good way to show it :thumbsup:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/jekyll/site.rb
@@ -209,17 +164,12 @@ def generate
# Returns nothing.
def render
payload = site_payload
- self.posts.each do |post|
- post.render(self.layouts, payload)
- end
-
- self.pages.each do |page|
- relative_permalinks_deprecation_method if page.uses_relative_permalinks
- page.render(self.layouts, payload)
+ files.each do |post|

maybe call the each inner varibale file will be better... I know, it was a ultra minimal smell... hehehe

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

@jpiasetz, still you there?

@parkr parkr closed this
@jpiasetz

@parkr this just too out of daete?

@parkr
Owner

Nope I deleted master by mistake. Sorry!

@parkr parkr reopened this
@jpiasetz

No worries haha. I probably should get back on fixing these up.

@parkr
Owner

Thanks @jpiasetz :)

@parkr
Owner

Also rebasing on master would be :sparkles:

jpiasetz added some commits
@jpiasetz jpiasetz Rename dest to the more descriptive and consistent destination ba2a569
@jpiasetz jpiasetz Extract config setup to loops e56948a
@jpiasetz jpiasetz Remove unecessary self. It keeps the code cleaner and also helps avoi…
…d accident shadowing.
218bf4a
@jpiasetz jpiasetz Move checks to init, FailFast don't wait around to say something is w…
…rong. Do it as soon as you can.
bc977c3
@jpiasetz jpiasetz Move relative_permalinks_deprecation into page. Whether or not a page…
… is using relative_permalinks is page's concern not site's.
c4c772c
@jpiasetz jpiasetz Move aggregate_post_info into post b5f8705
@jpiasetz jpiasetz Remove source from Post, Page, and StaticFile constructor (it's alrea…
…dy beening passed as part of site)
491a6bf
@jpiasetz jpiasetz Change arguement to path for Post, Draft, Page, StaticFile and Layout…
…. At the end of the chain read_yaml load off a path. Why split it up in site only to join it back together in the end? Also why is it site's responsibility to break up a path? Let the object break up the path how they want. Refactor the read methods into one to avoid recursion and modify read_yaml to take a path. The multi reads seemed to be a side effect of having to break the path up so join them back together.
96d7d13
@jpiasetz jpiasetz Combined the posts, pages, and statics into one array. Site's one res…
…ponsibilities should be dealing with the files not dealing with posts, pages, layouts, static_files,..., rss_files, images, etc. If more types get added would variables just keep getting made for them? Make the variables reflect responsibilities.
e41e831
@jpiasetz jpiasetz Extract parent object from posts and pages a3ceed0
@jpiasetz jpiasetz Removed lsi messages. Why is the lsi the only feature that gets to wr…
…ite to the console status updates by default? Is it because it's expected to take a long time? There's no way to turn it off and it could expected behaviour if jekyll is included somewhere else.
e7822e1
@jpiasetz jpiasetz Refactor read and extract conditional to exceptions on class. Extract…
… the conditional to the class - Tell Don't Ask, decision about the state of the object should be made in the objects themselves. It cut's down on coupling and makes it easier to change. If you use the Post class by outsite of site it should still tell you it can't be made with an invalid name.
daa384a
@jpiasetz

Bah this is in a big mess. On the plus side looks like lots of the stuff I was suggesting already has been done :+1:. I'll work through cleaning up my crappy rebase and then figure out what still applies.

@parkr
Owner

With collections, I'd eventually like to clean up Posts and Drafts so they are part of collections, just automatically read-in rather than requiring the user to specify them in their config. If we can work together to make Jekyll::Document the common interface here while allowing for specificity, then we can achieve something pretty darn cool.

@troyswanson

If we can work together to make Jekyll::Document the common interface

:heart_eyes: :heartbeat: :+1:

@jpiasetz

@parkr that's where you're going with #2199 right?

@parkr
Owner

@jpiasetz Documents will eventually be abstracted to posts and pages but not until 3.0.0, which will be a huuuuuge internal rewrite.

@parkr
Owner

@jpiasetz Would love your expertise on merging the pages/posts concepts into the collection concept. Ideally we'd :dog:food everything so pages and posts are just collections. Feeling a bit like Jekyll::Collection ought to be a subclass of Array right now, and that all pages/posts should be subclasses of Jekyll::Document.

@parkr
Owner

Let's revisit this in a different PR. :)

@parkr parkr closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 19, 2014
  1. @jpiasetz
  2. @jpiasetz
  3. @jpiasetz
  4. @jpiasetz

    Move checks to init, FailFast don't wait around to say something is w…

    jpiasetz authored
    …rong. Do it as soon as you can.
  5. @jpiasetz

    Move relative_permalinks_deprecation into page. Whether or not a page…

    jpiasetz authored
    … is using relative_permalinks is page's concern not site's.
  6. @jpiasetz
  7. @jpiasetz

    Remove source from Post, Page, and StaticFile constructor (it's alrea…

    jpiasetz authored
    …dy beening passed as part of site)
  8. @jpiasetz

    Change arguement to path for Post, Draft, Page, StaticFile and Layout…

    jpiasetz authored
    …. At the end of the chain read_yaml load off a path. Why split it up in site only to join it back together in the end? Also why is it site's responsibility to break up a path? Let the object break up the path how they want. Refactor the read methods into one to avoid recursion and modify read_yaml to take a path. The multi reads seemed to be a side effect of having to break the path up so join them back together.
  9. @jpiasetz

    Combined the posts, pages, and statics into one array. Site's one res…

    jpiasetz authored
    …ponsibilities should be dealing with the files not dealing with posts, pages, layouts, static_files,..., rss_files, images, etc. If more types get added would variables just keep getting made for them? Make the variables reflect responsibilities.
  10. @jpiasetz
  11. @jpiasetz

    Removed lsi messages. Why is the lsi the only feature that gets to wr…

    jpiasetz authored
    …ite to the console status updates by default? Is it because it's expected to take a long time? There's no way to turn it off and it could expected behaviour if jekyll is included somewhere else.
  12. @jpiasetz

    Refactor read and extract conditional to exceptions on class. Extract…

    jpiasetz authored
    … the conditional to the class - Tell Don't Ask, decision about the state of the object should be made in the objects themselves. It cut's down on coupling and makes it easier to change. If you use the Post class by outsite of site it should still tell you it can't be made with an invalid name.
Commits on Apr 11, 2014
  1. @jpiasetz

    Fix missed merge

    jpiasetz authored
This page is out of date. Refresh to see the latest.
View
1  lib/jekyll.rb
@@ -38,6 +38,7 @@ def require_all(path)
require 'jekyll/convertible'
require 'jekyll/url'
require 'jekyll/layout'
+require 'jekyll/item'
require 'jekyll/page'
require 'jekyll/post'
require 'jekyll/excerpt'
View
13 lib/jekyll/convertible.rb
@@ -34,23 +34,22 @@ def merged_file_read_opts(opts)
# Read the YAML frontmatter.
#
- # base - The String path to the dir containing the file.
- # name - The String filename of the file.
+ # path - The String path of the file.
# opts - optional parameter to File.read, default at site configs
#
# Returns nothing.
- def read_yaml(base, name, opts = {})
+ def read_yaml(path, opts ={})
begin
- self.content = File.read(File.join(base, name),
- merged_file_read_opts(opts))
+ self.content = File.read(path, merged_file_read_opts(opts))
+
if content =~ /\A(---\s*\n.*?\n?)^(---\s*$\n?)/m
self.content = $POSTMATCH
self.data = SafeYAML.load($1)
end
rescue SyntaxError => e
- puts "YAML Exception reading #{File.join(base, name)}: #{e.message}"
+ puts "YAML Exception reading #{path}: #{e.message}"
rescue Exception => e
- puts "Error reading file #{File.join(base, name)}: #{e.message}"
+ puts "Error reading file #{path}: #{e.message}"
end
self.data ||= {}
View
12 lib/jekyll/draft.rb
@@ -13,16 +13,6 @@ def self.valid?(name)
name =~ MATCHER
end
- # Get the full path to the directory containing the draft files
- def containing_dir(source, dir)
- File.join(source, dir, '_drafts')
- end
-
- # The path to the draft source file, relative to the site source
- def relative_path
- File.join(@dir, '_drafts', @name)
- end
-
# Extract information from the post filename.
#
# name - The String filename of the post file.
@@ -30,7 +20,7 @@ def relative_path
# Returns nothing.
def process(name)
m, slug, ext = *name.match(MATCHER)
- self.date = File.mtime(File.join(@base, name))
+ self.date = File.mtime(self.file_path)
self.slug = slug
self.ext = ext
end
View
4 lib/jekyll/generators/pagination.rb
@@ -43,10 +43,10 @@ def paginate(site, page)
(1..pages).each do |num_page|
pager = Pager.new(site, num_page, all_posts, pages)
if num_page > 1
- newpage = Page.new(site, site.source, page.dir, page.name)
+ newpage = Page.new(site, File.join(site.source, page.dir, page.name))
newpage.pager = pager
newpage.dir = Pager.paginate_path(site, num_page)
- site.pages << newpage
+ site.files << newpage
else
page.pager = pager
end
View
45 lib/jekyll/item.rb
@@ -0,0 +1,45 @@
+module Jekyll
+ class Item
+ include Convertible
+
+ attr_accessor :site, :pager, :name, :ext, :basename, :data, :content, :output, :file_path
+
+ # Initialize this Post instance.
+ #
+ # site - The Site.
+ # path - The String path of the post file.
+ #
+ # Returns the new Post.
+ def initialize(site, path)
+ @file_path = path
+ @site = site
+ @name = File.basename(path)
+ process(@name)
+ read_yaml(path)
+ end
+
+ # Add any necessary layouts to this post.
+ #
+ # layouts - A Hash of {"name" => "layout"}.
+ # site_payload - The site payload hash.
+ #
+ # Returns nothing.
+ def render(template, layouts, site_payload)
+ payload = template.deep_merge(site_payload)
+ do_layout(payload, layouts)
+ end
+
+ # The full path and filename of the post. Defined in the YAML of the post
+ # body.
+ #
+ # Returns the String permalink or nil if none has been set.
+ def permalink
+ data && data['permalink']
+ end
+
+ # Returns the object as a debug String.
+ def inspect
+ "#<Jekyll:#{self.class.name} @name=#{self.name.inspect}>"
+ end
+ end
+end
View
26 lib/jekyll/layout.rb
@@ -2,35 +2,21 @@ module Jekyll
class Layout
include Convertible
- # Gets the Site object.
- attr_reader :site
-
- # Gets the name of this layout.
- attr_reader :name
-
- # Gets/Sets the extension of this layout.
- attr_accessor :ext
-
- # Gets/Sets the Hash that holds the metadata for this layout.
- attr_accessor :data
-
- # Gets/Sets the content of this layout.
- attr_accessor :content
+ attr_reader :site, :name
+ attr_accessor :data, :content
# Initialize a new Layout.
#
# site - The Site.
- # base - The String path to the source.
- # name - The String filename of the post file.
- def initialize(site, base, name)
+ # path - The String path of the post file.
+ def initialize(site, path)
@site = site
- @base = base
- @name = name
+ @name = File.basename(path)
self.data = {}
process(name)
- read_yaml(base, name)
+ read_yaml(path)
end
# Extract information from the layout filename.
View
49 lib/jekyll/page.rb
@@ -1,11 +1,8 @@
module Jekyll
- class Page
- include Convertible
+ class Page < Item
attr_writer :dir
- attr_accessor :site, :pager
- attr_accessor :name, :ext, :basename
- attr_accessor :data, :content, :output
+ attr_accessor :pager, :basename
# Attributes for Liquid templates
ATTRIBUTES_FOR_LIQUID = %w[
@@ -22,14 +19,9 @@ class Page
# base - The String path to the source.
# dir - The String path between the source and the file.
# name - The String filename of the file.
- def initialize(site, base, dir, name)
- @site = site
- @base = base
- @dir = dir
- @name = name
-
- process(name)
- read_yaml(File.join(base, dir), name)
+ def initialize(site, path)
+ super(site, path)
+ @dir = File.dirname(path).sub(site.source, '')
end
# The generated directory into which the page will be placed
@@ -41,19 +33,6 @@ def dir
url[-1, 1] == '/' ? url : File.dirname(url)
end
- # The full path and filename of the post. Defined in the YAML of the post
- # body.
- #
- # Returns the String permalink or nil if none has been set.
- def permalink
- return nil if data.nil? || data['permalink'].nil?
- if site.config['relative_permalinks']
- File.join(@dir, data['permalink'])
- else
- data['permalink']
- end
- end
-
# The template of the permalink.
#
# Returns the template String.
@@ -109,12 +88,24 @@ def process(name)
#
# Returns nothing.
def render(layouts, site_payload)
- payload = Utils.deep_merge_hashes({
+ relative_permalinks_deprecation_method if uses_relative_permalinks
+ template = {
"page" => to_liquid,
'paginator' => pager.to_liquid
- }, site_payload)
+ }
+ super(template, layouts, site_payload)
+ end
- do_layout(payload, layouts)
+ def relative_permalinks_deprecation_method
+ if config['relative_permalinks'] && has_relative_page?
+ $stderr.puts # Places newline after "Generating..."
+ Jekyll.logger.warn "Deprecation:", "Starting in 2.0, permalinks for pages" +
+ " in subfolders must be relative to the" +
+ " site source directory, not the parent" +
+ " directory. Check http://jekyllrb.com/docs/upgrading/"+
+ " for more info."
+ $stderr.print Jekyll.logger.formatted_topic("") + "..." # for "done."
+ end
end
# The path to the source file
View
74 lib/jekyll/post.rb
@@ -1,7 +1,6 @@
module Jekyll
- class Post
+ class Post < Item
include Comparable
- include Convertible
# Valid post name regex.
MATCHER = /^(.+\/)*(\d+-\d+-\d+)-(.*)(\.[^.]+)$/
@@ -33,35 +32,41 @@ def self.valid?(name)
name =~ MATCHER
end
- attr_accessor :site
- attr_accessor :data, :extracted_excerpt, :content, :output, :ext
- attr_accessor :date, :slug, :tags, :categories
-
- attr_reader :name
+ attr_accessor :extracted_excerpt, :date, :slug, :published, :tags, :categories
# Initialize this Post instance.
#
# site - The Site.
- # base - The String path to the dir containing the post file.
- # name - The String filename of the post file.
+ # path - The String path of the post file.
#
# Returns the new Post.
- def initialize(site, source, dir, name)
- @site = site
- @dir = dir
- @base = containing_dir(source, dir)
- @name = name
-
- self.categories = dir.downcase.split('/').reject { |x| x.empty? }
- process(name)
- read_yaml(@base, name)
+ def initialize(site, path)
+ super(site, path)
+ raise NameError, "Invalid filename" unless self.class.valid?(@name)
+ @dir = File.dirname(path).sub(site.source, '').sub(/(_posts|_drafts)/, '')
+ self.categories = @dir.downcase.split('/').reject { |x| x.empty? }
+ process(@name)
+ read_yaml(path)
if data.has_key?('date')
self.date = Time.parse(data["date"].to_s)
end
+ published = self.published?
+
populate_categories
populate_tags
+ raise RangeError, "Not published" unless published? && (site.future || self.date <= site.time)
+
+ site.aggregate_post_info(self)
+ end
+
+ def published?
+ if self.data.has_key?('published') && self.data['published'] == false
+ false
+ else
+ true
+ end
end
def populate_categories
@@ -75,19 +80,14 @@ def populate_tags
self.tags = Utils.pluralized_array_from_hash(data, "tag", "tags").flatten
end
- # Get the full path to the directory containing the post files
- def containing_dir(source, dir)
- return File.join(source, dir, '_posts')
- end
-
# Read the YAML frontmatter.
#
# base - The String path to the dir containing the file.
# name - The String filename of the file.
#
# Returns nothing.
- def read_yaml(base, name)
- super(base, name)
+ def read_yaml(path)
+ super(path)
self.extracted_excerpt = extract_excerpt
end
@@ -167,14 +167,6 @@ def dir
File.dirname(url)
end
- # The full path and filename of the post. Defined in the YAML of the post
- # body (optional).
- #
- # Returns the String permalink.
- def permalink
- data && data['permalink']
- end
-
def template
case site.permalink_style
when :pretty
@@ -240,17 +232,11 @@ def related_posts(posts)
#
# Returns nothing.
def render(layouts, site_payload)
- # construct payload
- payload = Utils.deep_merge_hashes({
- "site" => { "related_posts" => related_posts(site_payload["site"]["posts"]) },
- "page" => to_liquid(EXCERPT_ATTRIBUTES_FOR_LIQUID)
- }, site_payload)
-
- if generate_excerpt?
- extracted_excerpt.do_layout(payload, {})
- end
-
- do_layout(payload.merge({"page" => to_liquid}), layouts)
+ template = {
+ "page" => self.to_liquid,
+ "site" => { "related_posts" => related_posts(site_payload["site"]["posts"]) }
+ }
+ super(template, layouts, site_payload)
end
# Obtain destination path.
View
3  lib/jekyll/related_posts.rb
@@ -28,15 +28,12 @@ def build
def build_index
self.class.lsi ||= begin
lsi = Classifier::LSI.new(:auto_rebuild => false)
- display("Populating LSI...")
site.posts.each do |x|
lsi.add_item(x)
end
- display("Rebuilding index...")
lsi.build_index
- display("")
lsi
end
end
View
166 lib/jekyll/site.rb
@@ -1,7 +1,7 @@
module Jekyll
class Site
attr_accessor :config, :layouts, :posts, :pages, :static_files,
- :categories, :exclude, :include, :source, :dest, :lsi, :highlighter,
+ :categories, :exclude, :include, :source, :destination, :lsi, :highlighter,
:permalink_style, :tags, :time, :future, :safe, :plugins, :limit_posts,
:show_drafts, :keep_files, :baseurl, :data, :file_read_opts, :gems
@@ -18,13 +18,17 @@ def initialize(config)
end
self.source = File.expand_path(config['source'])
- self.dest = File.expand_path(config['destination'])
+ self.destination = File.expand_path(config['destination'])
self.plugins = plugins_path
- self.permalink_style = config['permalink'].to_sym
self.file_read_opts = {}
self.file_read_opts[:encoding] = config['encoding'] if config['encoding']
+ if limit_posts < 0
+ raise ArgumentError, "limit_posts must be a non-negative number"
+ end
+
+ ensure_not_in_dest
reset
setup
end
@@ -47,24 +51,17 @@ def process
def reset
self.time = (config['time'] ? Time.parse(config['time'].to_s) : Time.now)
self.layouts = {}
- self.posts = []
- self.pages = []
+ self.files = []
self.static_files = []
self.categories = Hash.new { |hash, key| hash[key] = [] }
self.tags = Hash.new { |hash, key| hash[key] = [] }
self.data = {}
-
- if limit_posts < 0
- raise ArgumentError, "limit_posts must be a non-negative number"
- end
end
# Load necessary libraries, plugins, converters, and generators.
#
# Returns nothing.
def setup
- ensure_not_in_dest
-
# If safe mode is off, load in any Ruby files under the plugins
# directory.
unless safe
@@ -84,7 +81,7 @@ def setup
# Check that the destination dir isn't the source dir or a directory
# parent to the source dir.
def ensure_not_in_dest
- dest_pathname = Pathname.new(dest)
+ dest_pathname = Pathname.new(destination)
Pathname.new(source).ascend do |path|
if path == dest_pathname
raise FatalException.new "Destination directory cannot be or contain the Source directory."
@@ -119,102 +116,57 @@ def plugins_path
end
end
- # Read Site data from disk and load it into internal data structures.
+ # Recursively traverse source directory to find posts, pages, layouts
+ # and static filesthat will become part of the site according to the rules
+ # in filter_entries.
#
# Returns nothing.
def read
- self.layouts = LayoutReader.new(self).read
- read_directories
- read_data(config['data_source'])
- end
-
- # Recursively traverse directories to find posts, pages and static files
- # that will become part of the site according to the rules in
- # filter_entries.
- #
- # dir - The String relative path of the directory to read. Default: ''.
- #
- # Returns nothing.
- def read_directories(dir = '')
- base = File.join(source, dir)
- entries = Dir.chdir(base) { filter_entries(Dir.entries('.'), base) }
-
- read_posts(dir)
- read_drafts(dir) if show_drafts
- posts.sort!
- limit_posts! if limit_posts > 0 # limit the posts if :limit_posts option is set
-
- entries.each do |f|
- f_abs = File.join(base, f)
- if File.directory?(f_abs)
- f_rel = File.join(dir, f)
- read_directories(f_rel) unless dest.sub(/\/$/, '') == f_abs
- elsif has_yaml_header?(f_abs)
- page = Page.new(self, source, dir, f)
- pages << page if page.published?
- else
- static_files << StaticFile.new(self, source, dir, f)
+ filter_entries(Dir["#{source}/**/{*,.*}"]).each do |path|
+ begin
+ klass = if File.directory?(path)
+ nil
+ elsif path =~ /#{config['layouts']}/
+ ext = File.extname(path)
+ name = File.basename(path, ext)
+ layouts[name] = Layout.new(self, path)
+ nil
+ elsif path =~ /_posts/
+ Post
+ elsif path =~ /_drafts/ && show_drafts
+ Draft
+ elsif path.sub(source, '') =~ /\/_/
+ nil
+ elsif has_yaml_header?(path)
+ Page
+ else
+ StaticFile
+ end

I do not like these big chained if expressions
Maybe this can be better like this:

if File.file?(path) && path.sub(source, '') !~ /\/_/
  klass = if path =~ /_posts/
    Post
  elsif path =~ /_drafts/ && show_drafts
    Draft
  elsif has_yaml_header?(path)
    Page
  else
    StaticFile
  end
  files << klass.new(self, path)
elsif path =~ /#{config['layouts']}/
   ext = File.extname(path)
   name = File.basename(path, ext)
   layouts[name] = Layout.new(self, path)
end   

It will reduce a little the cyclomatic complexity of this statement (I'd manually calculated it, it became 2 paths less than the old one), and makes the code more simple to understand in my opinion...

And maybe this can be increased with a factory method for the first if statement

Yea I think this wants to be a factory. I just wasn't sure how to get it all the way. This is a good way to show it :thumbsup:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ files << klass.new(self, path) if klass
+ rescue NameError
+ Jekyll.logger.warn "Warning:", "Invalid filename on #{path}"
+ rescue RangeError
end
end
-
- pages.sort_by!(&:name)
+ posts.sort!
+ limit_posts! if limit_posts > 0 # limit the posts if :limit_posts option is set
end
- # Read all the files in <source>/<dir>/_posts and create a new Post
- # object with each one.
- #
- # dir - The String relative path of the directory to read.
- #
- # Returns nothing.
- def read_posts(dir)
- posts = read_content(dir, '_posts', Post)
-
- posts.each do |post|
- if post.published? && (future || post.date <= time)
- aggregate_post_info(post)
- end
- end
- end
-
- # Read all the files in <source>/<dir>/_drafts and create a new Post
- # object with each one.
- #
- # dir - The String relative path of the directory to read.
- #
- # Returns nothing.
- def read_drafts(dir)
- drafts = read_content(dir, '_drafts', Draft)
-
- drafts.each do |draft|
- aggregate_post_info(draft)
- end
+ def posts
+ files.select { |file| file.is_a?Post }.sort
end
- def read_content(dir, magic_dir, klass)
- get_entries(dir, magic_dir).map do |entry|
- klass.new(self, source, dir, entry) if klass.valid?(entry)
- end.reject do |entry|
- entry.nil?
- end
+ def posts=(posts)
+ files.delete_if { |file| file.is_a?Post }
+ files.concat(posts)
end
- # Read and parse all yaml files under <source>/<dir>
- #
- # Returns nothing
- def read_data(dir)
- base = File.join(source, dir)
- return unless File.directory?(base) && (!safe || !File.symlink?(base))
-
- entries = Dir.chdir(base) { Dir['*.{yaml,yml}'] }
- entries.delete_if { |e| File.directory?(File.join(base, e)) }
-
- entries.each do |entry|
- path = File.join(source, dir, entry)
- next if File.symlink?(path) && safe
+ def pages
+ files.select { |file| file.is_a?Page }
+ end
- key = sanitize_filename(File.basename(entry, '.*'))
- self.data[key] = SafeYAML.load_file(path)
- end
+ def static_files
+ files.select { |file| file.is_a?StaticFile }
end
# Run each of the Generators.
@@ -254,7 +206,7 @@ def cleanup
#
# Returns nothing.
def write
- each_site_file { |item| item.write(dest) }
+ files.each { |item| item.write(destination) }
end
# Construct a Hash of Posts indexed by the specified Post attribute.
@@ -325,6 +277,14 @@ def filter_entries(entries, base_directory = nil)
EntryFilter.new(self, base_directory).filter(entries)
end
+ def filter_exclude?(entry)
+ exclude.glob_include?(entry.sub("#{source}/", ''))
+ end
+
+ def filter_include?(entry)
+ include.glob_include?(entry.sub("#{source}/", ''))
+ end
+
# Get the implementation class for the given Converter.
#
# klass - The Class of the Converter to fetch.
@@ -378,18 +338,6 @@ def aggregate_post_info(post)
post.tags.each { |c| tags[c] << post }
end
- def relative_permalinks_deprecation_method
- if config['relative_permalinks'] && has_relative_page?
- $stderr.puts # Places newline after "Generating..."
- Jekyll.logger.warn "Deprecation:", "Starting in 2.0, permalinks for pages" +
- " in subfolders must be relative to the" +
- " site source directory, not the parent" +
- " directory. Check http://jekyllrb.com/docs/upgrading/"+
- " for more info."
- $stderr.print Jekyll.logger.formatted_topic("") + "..." # for "done."
- end
- end
-
def each_site_file
%w(posts pages static_files).each do |type|
send(type).each do |item|
View
22 lib/jekyll/static_file.rb
@@ -1,24 +1,16 @@
module Jekyll
class StaticFile
+ attr_reader :path
# The cache of last modification times [path] -> mtime.
@@mtimes = Hash.new
# Initialize a new StaticFile.
#
# site - The Site.
- # base - The String path to the <source>.
- # dir - The String path between <source> and the file.
- # name - The String filename of the file.
- def initialize(site, base, dir, name)
+ # path - The File path.
+ def initialize(site, path)
@site = site
- @base = base
- @dir = dir
- @name = name
- end
-
- # Returns source file path.
- def path
- File.join(@base, @dir, @name)
+ @path = path
end
# Returns the source file path relative to the site source
@@ -32,7 +24,7 @@ def relative_path
#
# Returns destination file path.
def destination(dest)
- File.join(dest, @dir, @name)
+ path.sub(@site.source, dest)
end
# Returns last modification time for this file.
@@ -47,6 +39,10 @@ def modified?
@@mtimes[path] != mtime
end
+ def render(layout, payload)
+ #Quack
+ end
+
# Write the static file to the destination directory (if modified).
#
# dest - The String path to the destination dir.
View
19 test/test_convertible.rb
@@ -10,36 +10,39 @@ class TestConvertible < Test::Unit::TestCase
end
should "parse the front-matter correctly" do
- ret = @convertible.read_yaml(@base, 'front_matter.erb')
+ path = File.join(@base, 'front_matter.erb')
+ ret = @convertible.read_yaml(path)
assert_equal({'test' => 'good'}, ret)
end
should "not parse if the front-matter is not at the start of the file" do
- ret = @convertible.read_yaml(@base, 'broken_front_matter1.erb')
+ path = File.join(@base, 'broken_front_matter1.erb')
+ ret = @convertible.read_yaml(path)
assert_equal({}, ret)
end
should "not parse if there is syntax error in front-matter" do
- name = 'broken_front_matter2.erb'
+ path = File.join(@base, 'broken_front_matter2.erb')
out = capture_stdout do
- ret = @convertible.read_yaml(@base, name)
+ ret = @convertible.read_yaml(path)
assert_equal({}, ret)
end
assert_match(/YAML Exception|syntax error|Error reading file/, out)
- assert_match(/#{File.join(@base, name)}/, out)
+ assert_match(/#{path}/, out)
end
should "not allow ruby objects in yaml" do
+ path = File.join(@base, 'exploit_front_matter.erb')
out = capture_stdout do
- @convertible.read_yaml(@base, 'exploit_front_matter.erb')
+ @convertible.read_yaml(path)
end
assert_no_match /undefined class\/module DoesNotExist/, out
end
should "not parse if there is encoding error in file" do
- name = 'broken_front_matter3.erb'
+ path = File.join(@base, 'broken_front_matter3.erb')
out = capture_stdout do
- ret = @convertible.read_yaml(@base, name, :encoding => 'utf-8')
+ ret = @convertible.read_yaml(path, :encoding => 'utf-8')
assert_equal({}, ret)
end
assert_match(/invalid byte sequence in UTF-8/, out)
View
2  test/test_generated_site.rb
@@ -22,7 +22,7 @@ class TestGeneratedSite < Test::Unit::TestCase
end
should "render latest post's content" do
- assert @index.include?(@site.posts.last.content)
+ assert @index.include?(@site.posts.last.content.gsub(/<[^>]*>/ui,''))
end
should "hide unpublished posts" do
View
36 test/test_page.rb
@@ -1,14 +1,15 @@
require 'helper'
class TestPage < Test::Unit::TestCase
- def setup_page(*args)
- dir, file = args
- dir, file = ['', dir] if file.nil?
- @page = Page.new(@site, source_dir, dir, file)
+ def setup_page(dir, file)
+ path = File.join(source_dir, dir, file)
+ @site.source = source_dir
+ @page = Page.new(@site, path)
end
def do_render(page)
- layouts = { "default" => Layout.new(@site, source_dir('_layouts'), "simple.html")}
+ path = File.join(source_dir('_layouts'), "simple.html")
+ layouts = { "default" => Layout.new(@site, path)}
page.render(layouts, {"site" => {"posts" => []}})
end
@@ -21,7 +22,7 @@ def do_render(page)
context "processing pages" do
should "create url based on filename" do
- @page = setup_page('contacts.html')
+ @page = setup_page('', 'contacts.html')
assert_equal "/contacts.html", @page.url
end
@@ -43,12 +44,12 @@ def do_render(page)
end
should "deal properly with extensions" do
- @page = setup_page('deal.with.dots.html')
+ @page = setup_page('', 'deal.with.dots.html')
assert_equal ".html", @page.ext
end
should "deal properly with dots" do
- @page = setup_page('deal.with.dots.html')
+ @page = setup_page('', 'deal.with.dots.html')
assert_equal "deal.with.dots", @page.basename
end
@@ -81,12 +82,12 @@ def do_render(page)
end
should "return dir correctly" do
- @page = setup_page('contacts.html')
+ @page = setup_page('', 'contacts.html')
assert_equal '/contacts/', @page.dir
end
should "return dir correctly for index page" do
- @page = setup_page('index.html')
+ @page = setup_page('', 'index.html')
assert_equal '/', @page.dir
end
@@ -116,14 +117,13 @@ def do_render(page)
context "with any other url style" do
should "return dir correctly" do
@site.permalink_style = nil
- @page = setup_page('contacts.html')
+ @page = setup_page('', 'contacts.html')
assert_equal '/', @page.dir
end
end
should "respect permalink in yaml front matter" do
- file = "about.html"
- @page = setup_page(file)
+ @page = setup_page('', 'about.html')
assert_equal "/about/", @page.permalink
assert_equal @page.permalink, @page.url
@@ -143,7 +143,7 @@ def do_render(page)
context "with specified layout of nil" do
setup do
- @page = setup_page('sitemap.xml')
+ @page = setup_page('', 'sitemap.xml')
end
should "layout of nil is respected" do
@@ -157,7 +157,7 @@ def do_render(page)
end
should "write properly" do
- page = setup_page('contacts.html')
+ page = setup_page('', 'contacts.html')
do_render(page)
page.write(dest_dir)
@@ -175,7 +175,7 @@ def do_render(page)
end
should "write properly without html extension" do
- page = setup_page('contacts.html')
+ page = setup_page('', 'contacts.html')
page.site.permalink_style = :pretty
do_render(page)
page.write(dest_dir)
@@ -185,7 +185,7 @@ def do_render(page)
end
should "write properly with extension different from html" do
- page = setup_page("sitemap.xml")
+ page = setup_page('', 'sitemap.xml')
page.site.permalink_style = :pretty
do_render(page)
page.write(dest_dir)
@@ -197,7 +197,7 @@ def do_render(page)
end
should "write dotfiles properly" do
- page = setup_page('.htaccess')
+ page = setup_page('', '.htaccess')
do_render(page)
page.write(dest_dir)
View
24 test/test_post.rb
@@ -2,11 +2,13 @@
class TestPost < Test::Unit::TestCase
def setup_post(file)
- Post.new(@site, source_dir, '', file)
+ path = File.join(source_dir, '_posts', file)
+ @site.source = source_dir
+ Post.new(@site, path)
end
def do_render(post)
- layouts = { "default" => Layout.new(@site, source_dir('_layouts'), "simple.html")}
+ layouts = { "default" => Layout.new(@site, File.join(source_dir('_layouts'), "simple.html"))}
post.render(layouts, {"site" => {"posts" => []}})
end
@@ -96,7 +98,7 @@ def do_render(post)
should "respect permalink in yaml front matter" do
file = "2008-12-03-permalinked-post.textile"
@post.process(file)
- @post.read_yaml(@source, file)
+ @post.read_yaml(File.join(@source, file))
assert_equal "my_category/permalinked-post", @post.permalink
assert_equal "/my_category", @post.dir
@@ -120,7 +122,7 @@ def do_render(post)
@source = source_dir('win/_posts')
end
should "read yaml front-matter" do
- @post.read_yaml(@source, @real_file)
+ @post.read_yaml(File.join(@source, @real_file))
assert_equal({"title" => "Test title", "layout" => "post", "tag" => "Ruby"}, @post.data)
assert_equal "This is the content", @post.content
@@ -132,7 +134,7 @@ def do_render(post)
@real_file = "2010-01-08-triple-dash.markdown"
end
should "consume the embedded dashes" do
- @post.read_yaml(@source, @real_file)
+ @post.read_yaml(File.join(@source, @real_file))
assert_equal({"title" => "Foo --- Bar"}, @post.data)
assert_equal "Triple the fun!", @post.content
@@ -289,7 +291,7 @@ def do_render(post)
end
should "read yaml front-matter" do
- @post.read_yaml(@source, @real_file)
+ @post.read_yaml(File.join(@source, @real_file))
assert_equal({"title" => "Foo Bar", "layout" => "default"}, @post.data)
assert_equal "h1. {{ page.title }}\n\nBest *post* ever", @post.content
@@ -297,7 +299,7 @@ def do_render(post)
should "transform textile" do
@post.process(@real_file)
- @post.read_yaml(@source, @real_file)
+ @post.read_yaml(File.join(@source, @real_file))
@post.transform
assert_equal "<h1>{{ page.title }}</h1>\n<p>Best <strong>post</strong> ever</p>", @post.content
@@ -309,6 +311,7 @@ def do_render(post)
@post = setup_post(file)
@post.process(file)
@post.read_yaml(@source, file)
+ @post.read_yaml(File.join(@source, file))
do_render(@post)
end
@@ -334,7 +337,7 @@ def do_render(post)
@post.site.config['excerpt_separator'] = "\n---\n"
@post.process(file)
- @post.read_yaml(@source, file)
+ @post.read_yaml(File.join(@source, file))
@post.transform
end
@@ -564,8 +567,9 @@ def do_render(post)
end
should "generate categories and topics" do
- post = Post.new(@site, File.join(File.dirname(__FILE__), *%w[source]), 'foo', 'bar/2008-12-12-topical-post.textile')
- assert_equal ['foo'], post.categories
+ @site.source = File.join(File.dirname(__FILE__), *%w[source])
+ post = Post.new(@site, File.join(@site.source, 'foo', '_posts', 'bar/2008-12-12-topical-post.textile'))
+ assert_equal ['foo', 'bar'], post.categories
end
end
View
12 test/test_site.rb
@@ -97,7 +97,7 @@ def generate(site)
@site.process
some_static_file = @site.static_files[0].path
- dest = File.expand_path(@site.static_files[0].destination(@site.dest))
+ dest = File.expand_path(@site.static_files[0].destination(@site.destination))
mtime1 = File.stat(dest).mtime.to_i # first run must generate dest file
# need to sleep because filesystem timestamps have best resolution in seconds
@@ -126,7 +126,7 @@ def generate(site)
@site.process
some_static_file = @site.static_files[0].path
- dest = File.expand_path(@site.static_files[0].destination(@site.dest))
+ dest = File.expand_path(@site.static_files[0].destination(@site.destination))
mtime1 = File.stat(dest).mtime.to_i # first run must generate dest file
# need to sleep because filesystem timestamps have best resolution in seconds
@@ -180,8 +180,8 @@ def generate(site)
end
should "read posts" do
- @site.read_posts('')
- posts = Dir[source_dir('_posts', '**', '*')]
+ @site.read
+ posts = Dir[source_dir('**','_posts', '**', '*')]
posts.delete_if { |post| File.directory?(post) && !Post.valid?(post) }
assert_equal posts.size - @num_invalid_posts, @site.posts.size
end
@@ -200,11 +200,11 @@ def generate(site)
posts = Dir[source_dir("**", "_posts", "**", "*")]
posts.delete_if { |post| File.directory?(post) && !Post.valid?(post) }
- categories = %w(2013 bar baz category foo z_category publish_test win).sort
+ categories = %w(2013 bar baz category es foo z_category publish_test win).sort
assert_equal posts.size - @num_invalid_posts, @site.posts.size
assert_equal categories, @site.categories.keys.sort
- assert_equal 5, @site.categories['foo'].size
+ assert_equal 4, @site.categories['foo'].size
end
context 'error handling' do
View
10 test/test_tags.rb
@@ -13,7 +13,7 @@ def create_post(content, override = {}, converter_class = Jekyll::Converters::Ma
site = Site.new(Jekyll.configuration)
if override['read_posts']
- site.read_posts('')
+ site.read
end
info = { :filters => [Jekyll::Filters], :registers => { :site => site } }
@@ -218,8 +218,8 @@ def fill_post(code, override = {})
- 1 {% post_url 2008-11-21-complex %}
- 2 {% post_url /2008-11-21-complex %}
-- 3 {% post_url es/2008-11-21-nested %}
-- 4 {% post_url /es/2008-11-21-nested %}
+- 3 {% post_url 2008-11-21-nested %}
+- 4 {% post_url /2008-11-21-nested %}
CONTENT
create_post(content, {'permalink' => 'pretty', 'source' => source_dir, 'destination' => dest_dir, 'read_posts' => true})
end
@@ -234,8 +234,8 @@ def fill_post(code, override = {})
end
should "have the url to the \"nested\" post from 2008-11-21" do
- assert_match %r{3\s/2008/11/21/nested/}, @result
- assert_match %r{4\s/2008/11/21/nested/}, @result
+ assert_match %r{3\s/es/2008/11/21/nested/}, @result
+ assert_match %r{4\s/es/2008/11/21/nested/}, @result
end
end
Something went wrong with that request. Please try again.