Extract `Site#filter_entries` into a method object #1697

Merged
merged 2 commits into from Nov 5, 2013

Projects

None yet

2 participants

@mattr-
Member
mattr- commented Nov 5, 2013

Related to #1572, this pulls the hunk of code that's in Site#filter_entries into its own object using the method object pattern and simplifies it a great deal. The filter_entries API in the Site class is kept in place in order to maintain backwards compatibility.

@parkr parkr commented on the diff Nov 5, 2013
lib/jekyll/entry_filter.rb
+ entries.reject do |e|
+ unless included?(e)
+ special?(e) || backup?(e) || excluded?(e) || symlink?(e)
+ end
+ end
+ end
+
+ def included?(entry)
+ site.include.glob_include?(entry)
+ end
+
+ def special?(entry)
+ ['.', '_', '#'].include?(entry[0..0])
+ end
+
+ def backup?(entry)
@parkr
parkr Nov 5, 2013 Member

vim_poop? would be funny. This is probably better though.

@mattr-
mattr- Nov 5, 2013 Member

Too bad that method names can't be emoji, then I could have just used 💩

😆

@parkr parkr commented on the diff Nov 5, 2013
test/test_entry_filter.rb
@@ -0,0 +1,74 @@
+require 'helper'
+
+class TestEntryFilter < Test::Unit::TestCase
+ context "Filtering entries" do
+ setup do
+ stub(Jekyll).configuration do
+ Jekyll::Configuration::DEFAULTS.merge({'source' => source_dir, 'destination' => dest_dir})
+ end
+ @site = Site.new(Jekyll.configuration)
+ end
+
+ should "filter entries" do
+ ent1 = %w[foo.markdown bar.markdown baz.markdown #baz.markdown#
@parkr
parkr Nov 5, 2013 Member

Want to also test files in subdirs here? So if I have baz/#foo.markdown it'll still ignore?

@mattr-
mattr- Nov 5, 2013 Member

These are just the same tests extracted from test_site.rb so I'll make a note to add one for subdirectories.

@mattr-
mattr- Nov 5, 2013 Member

I tried to add this quickly and it failed. Considering how filter_entries is used currently - there is a Dir.chdir into every directory we want to filter entries from - should the lack of this extra test case block merging this in?

@parkr
parkr Nov 5, 2013 Member

If it is only meant to filter entries in the current directory, then I'm OK with it.

We could also do a entry.split($\).last to get the entries if it contains the $\ character. That is altering the behavior though so I'd probably say just leave it as-is and :shipit:.

@parkr
Member
parkr commented Nov 5, 2013

All the yes. 👍 Just a couple comments.

@mattr- mattr- merged commit a05e8af into master Nov 5, 2013

1 check passed

default The Travis CI build passed
Details
@mattr- mattr- deleted the entry-filter-method-object branch Nov 5, 2013
@mattr- mattr- added a commit that referenced this pull request Nov 5, 2013
@mattr- mattr- Update history to reflect merge of #1697 0a0acaa
@csim csim added a commit to csim/jekyll that referenced this pull request Nov 7, 2013
@mattr- @csim mattr- + csim Update history to reflect merge of #1697 2aac03b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment