Extract reading of data from `site.rb` to reduce responsibilities. #3545

Merged
merged 25 commits into from Mar 24, 2015

Conversation

Projects
None yet
6 participants
@MartinRogalla
Contributor

MartinRogalla commented Mar 4, 2015

I believe that the site.rb class right now has too many responsibilities. In this pull request I want to try and take out all the data reading tasks. I took out every single method that I found to have the reading of data responsibility and put it in a new class called Reader(reader.rb).

My methodology per method:

  1. Copy method from site.rb and fix references inside of the method.
  2. Redirect old method to new method in reader.rb
  3. Check if the test suite is passing
  4. Update all references to the old site.rb method and refer them to the new reader.rb method.
  5. Run the test suite
  6. Commit

Do you think this is a good idea at all or would you like it to be done differently?

I'm open to any criticism you might have.

Note: The usage of quotes is probably incorrect, I will fix these as soon as I know which style I should adhere to.

MartinRogalla added some commits Mar 4, 2015

Extracted `in_source_dir` from site.rb and into reader.rb.
Extracted `in_source_dir` from site.rb into reader.rb.
Updated all the references and tests.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `in_dest_dir` from site.rb into reader.rb
 - Extracted in_dest_dir from site.rb.
 - Updated References.
 - Ran Tests.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `filter_entries` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `get_entries` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_data_file` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `sanitize_filename` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_data_to` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_data` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_content` from site.rb into reader.rb
 - Extracted
 - Updated References
 - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `aggregate_post_info` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_drafts` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `read_posts` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Extracted `limit_posts` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com
Extracted `read_directories` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com
Extracted `read_collections` from site.rb into reader.rb
  - Extracted
  - Updated References
  - Ran Tests

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Made the Reader responsible for the actual Reading.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Added documentation, made method private and fixed quotes.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Added and improved documentation, fixed long method name.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>

@MartinRogalla MartinRogalla changed the title from Extract reading of data from `site.rb` to narrow responsibilities. to Extract reading of data from `site.rb` to reduce responsibilities. Mar 4, 2015

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 4, 2015

Contributor

Hmm, I didn't see that the cucumber test failed. Working on it right now.

Contributor

MartinRogalla commented Mar 4, 2015

Hmm, I didn't see that the cucumber test failed. Working on it right now.

Added site reference for encoding configuration.
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 4, 2015

Contributor

Should be working now.

Contributor

MartinRogalla commented Mar 4, 2015

Should be working now.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 5, 2015

Member

I'd rather not move in_source_dir and in_dest_dir out – can you add a def delegator for that method? Otherwise we'd have to update a large number of plugins...

Member

parkr commented Mar 5, 2015

I'd rather not move in_source_dir and in_dest_dir out – can you add a def delegator for that method? Otherwise we'd have to update a large number of plugins...

Moved the in_(source/dest)_dir back to site.rb.
After carefully looking at these two methods, as of right now they do not
belong in the reader, as they should also be used by the writer. Thus the
decision was made to move them back into the class containing the source
and dest fields, site.rb.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 5, 2015

Contributor

After looking at those methods a bit more carefully I spotted that in_(source/dest)_dir is used by multiple classes and not just the reader. Therefore I moved those two functions back to site.rb. Solves the problem @parkr mentioned as well.

Contributor

MartinRogalla commented Mar 5, 2015

After looking at those methods a bit more carefully I spotted that in_(source/dest)_dir is used by multiple classes and not just the reader. Therefore I moved those two functions back to site.rb. Solves the problem @parkr mentioned as well.

MartinRogalla added some commits Mar 6, 2015

Extracted draft, post reader into external classes.
Organized the draft, post and layout reader into the *readers* classes.
Fixed all references and ran tests.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Separated some more readers from the main reader.rb file.
 - Draft Reader
 - Collection Reader
 - Page Reader
 - Post Reader
 - Static File Reader

Fixed references and ran tests.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
Merge branch 'master' into site_extract_readers
Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 6, 2015

Contributor

Separated some more reader components into their own classes. I'm looking for some feedback on my progress. Don't hesitate to give me some critique ;)

Contributor

MartinRogalla commented Mar 6, 2015

Separated some more reader components into their own classes. I'm looking for some feedback on my progress. Don't hesitate to give me some critique ;)

lib/jekyll.rb
+ autoload :DraftReader, 'jekyll/readers/dynamic/draft_reader'
+ autoload :PostReader, 'jekyll/readers/dynamic/post_reader'
+ autoload :PageReader, 'jekyll/readers/dynamic/page_reader'
+ autoload :StaticFileReader, 'jekyll/readers/static/static_file_reader'

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

Please align all the strings vertically 😄

@parkr

parkr Mar 18, 2015

Member

Please align all the strings vertically 😄

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

Haha, sorry. Will be fixed.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

Haha, sorry. Will be fixed.

+ @site.layouts = LayoutReader.new(site).read
+ read_directories
+ @site.data = DataReader.new(site).read(site.config['data_source'])
+ CollectionReader.new(site).read

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

why not sticking this into @site.collections?

@parkr

parkr Mar 18, 2015

Member

why not sticking this into @site.collections?

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

I don't understand. What do you mean? Do you mean to re-read the collections every time the @site.collections is called?

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

I don't understand. What do you mean? Do you mean to re-read the collections every time the @site.collections is called?

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

The other readers have their output assigned, whereas CollectionReader manipulates @site.collections directly. That's probably a function of how collections are written more than anything else, but it does break the pattern in this code.

@mattr-

mattr- Mar 20, 2015

Member

The other readers have their output assigned, whereas CollectionReader manipulates @site.collections directly. That's probably a function of how collections are written more than anything else, but it does break the pattern in this code.

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.posts += PostReader.new(site).read('')

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

I'm not a big fan of += generally... hm.

@parkr

parkr Mar 18, 2015

Member

I'm not a big fan of += generally... hm.

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

I replaced all the += occurrences with concatenations. Apparently += creates a new array every time it is used, while concat simply appends the new elements to the old array.

Reference

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

I replaced all the += occurrences with concatenations. Apparently += creates a new array every time it is used, while concat simply appends the new elements to the old array.

Reference

This comment has been minimized.

@envygeeks

envygeeks Mar 19, 2015

Contributor

In the context of optimization (as you implied) that matters very little to Jekyll (as in the type of app we are not the type of programmers we are but, really, it should be applied to both.) We are a short lived process and our GC time matters little. We probably don't even hit the exponential scale required of caring about the GC time and whether or not we are creating new array's and marking old objects causing hefty GC time.

@envygeeks

envygeeks Mar 19, 2015

Contributor

In the context of optimization (as you implied) that matters very little to Jekyll (as in the type of app we are not the type of programmers we are but, really, it should be applied to both.) We are a short lived process and our GC time matters little. We probably don't even hit the exponential scale required of caring about the GC time and whether or not we are creating new array's and marking old objects causing hefty GC time.

site/_docs/history.md
@@ -191,6 +191,7 @@ permalink: "/docs/history/"
- Allow Travis to 'parallelize' our tests ([#2859]({{ site.repository }}/issues/2859))
- Fix test for Liquid rendering in Sass ([#2856]({{ site.repository }}/issues/2856))
- Fixing "vertycal" typo in site template's `_base.scss` ([#2889]({{ site.repository }}/issues/2889))
+- Convert remaining textile test documents to markdown ([#3528]({{ site.repository }}/issues/3528))

This comment has been minimized.

@parkr

parkr Mar 18, 2015

Member

? is this from a rebase? haha

@parkr

parkr Mar 18, 2015

Member

? is this from a rebase? haha

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

Yes, sorry about that. Haha.

@MartinRogalla

MartinRogalla Mar 19, 2015

Contributor

Yes, sorry about that. Haha.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 18, 2015

Member

I'm generally pretty happy with this, actually. Nice work.

/cc @jekyll/core, what do you think?

Member

parkr commented Mar 18, 2015

I'm generally pretty happy with this, actually. Nice work.

/cc @jekyll/core, what do you think?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 19, 2015

Contributor

I have no problems with this. 👍

Contributor

envygeeks commented Mar 19, 2015

I have no problems with this. 👍

Added corrections as suggested by @parkr.
 - Replaced occurrences of #array += with concat
   operations.(performance)
 - Corrected alignment.
 - Removed rebase artifact.

Signed-off-by: Martin Rogalla <martin@martinrogalla.com>
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 20, 2015

Member

One last set of eyes. @mattr-, would you mind taking a look?

Member

parkr commented Mar 20, 2015

One last set of eyes. @mattr-, would you mind taking a look?

lib/jekyll.rb
+ autoload :DraftReader, 'jekyll/readers/dynamic/draft_reader'
+ autoload :PostReader, 'jekyll/readers/dynamic/post_reader'
+ autoload :PageReader, 'jekyll/readers/dynamic/page_reader'
+ autoload :StaticFileReader, 'jekyll/readers/static/static_file_reader'

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Why the distinction between dynamic vs. static? Why add that level of indirection? If we had more static readers, I wouldn't be asking about this, but it feels like YAGNI for just the one.

@mattr-

mattr- Mar 20, 2015

Member

Why the distinction between dynamic vs. static? Why add that level of indirection? If we had more static readers, I wouldn't be asking about this, but it feels like YAGNI for just the one.

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Good point. I thought there were going to be more static readers. Therefore initially I split it up in the two directories. But yes you're right it's a bit overdone like this. I'll change it now.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Good point. I thought there were going to be more static readers. Therefore initially I split it up in the two directories. But yes you're right it's a bit overdone like this. I'll change it now.

lib/jekyll/reader.rb
+ def read_directories(dir = '')
+ retrieve_posts(dir)
+
+ # Obtain sub-directories in order to recursively read them.

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Comments in methods indicate to me code that is need of further refactoring towards expressiveness. What about a more functional style approach to filtering out the directories?

@mattr-

mattr- Mar 20, 2015

Member

Comments in methods indicate to me code that is need of further refactoring towards expressiveness. What about a more functional style approach to filtering out the directories?

lib/jekyll/reader.rb
+
+ dot_files = (dot - dot_dirs)
+
+ # Obtain all the pages.

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Same suggestion with the comment as above.

@mattr-

mattr- Mar 20, 2015

Member

Same suggestion with the comment as above.

+
+ # Retrieves all the posts(posts/drafts) from the given directory
+ # and add them to the site and sort them.
+ #

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

don't forget to document variables in your TomDoc. 😃

@mattr-

mattr- Mar 20, 2015

Member

don't forget to document variables in your TomDoc. 😃

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

lib/jekyll/site.rb
@@ -300,7 +146,7 @@ def generate
#
# Returns nothing.
def render
- relative_permalinks_deprecation_method
+ deprecated_rel_permalink

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

deprecated_relative_permalinks or relative_permalinks_are_deprecated are better name suggestions here, IMHO.

@mattr-

mattr- Mar 20, 2015

Member

deprecated_relative_permalinks or relative_permalinks_are_deprecated are better name suggestions here, IMHO.

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

lib/jekyll/site.rb
- # Aggregate post information
- #
- # post - The Post object to aggregate information for
+ # Warns the user if permanent links are relative tot the parent

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

typo here: to instead of tot 😺

@mattr-

mattr- Mar 20, 2015

Member

typo here: to instead of tot 😺

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Fixed.

@@ -479,16 +295,23 @@ def relative_permalinks_deprecation_method
end
end
+ # Get the to be written documents
+ #
+ # Returns an Array of Documents which should be written

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Love the new TomDoc! ❤️

@mattr-

mattr- Mar 20, 2015

Member

Love the new TomDoc! ❤️

def docs_to_write
documents.select(&:write?)
end
+ # Get all the documents
+ #
+ # Returns an Array of all Documents

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

And here too! 💜

@mattr-

mattr- Mar 20, 2015

Member

And here too! 💜

@@ -19,7 +19,7 @@ class TestEntryFilter < JekyllUnitTest
files = %w[index.html site.css .htaccess vendor]
@site.exclude = excludes + ["exclude*"]
- assert_equal files, @site.filter_entries(excludes + files + ["excludeA"])
+ assert_equal files, @site.reader.filter_entries(excludes + files + ["excludeA"])

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Can we not just instantiate a reader directly in these tests?

@mattr-

mattr- Mar 20, 2015

Member

Can we not just instantiate a reader directly in these tests?

This comment has been minimized.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

This would require decoupling of the Reader and EntryFilter from the site. I do think this should be done, but should be left for future work, as it is quite some work and this pull request will become too large in my opinion.

@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

This would require decoupling of the Reader and EntryFilter from the site. I do think this should be done, but should be left for future work, as it is quite some work and this pull request will become too large in my opinion.

This comment has been minimized.

@mattr-

mattr- Mar 20, 2015

Member

Agreed. Was just something to think about for the future. 😃

@mattr-

mattr- Mar 20, 2015

Member

Agreed. Was just something to think about for the future. 😃

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 20, 2015

Member

This is great stuff. I see places where you could push the refactoring further, but I think more pull requests can do that vs. holding this one up. I wrote a few questions, but I don't think they block merging this in.

I know you ended up moving stuff around quite a bit, but there's a bit of unnecessary noise in the diff (like the moving around of limit_posts for example). Not a huge issue, but something to be aware of, as that can make things harder to review.

All in all, I say :shipit:

Member

mattr- commented Mar 20, 2015

This is great stuff. I see places where you could push the refactoring further, but I think more pull requests can do that vs. holding this one up. I wrote a few questions, but I don't think they block merging this in.

I know you ended up moving stuff around quite a bit, but there's a bit of unnecessary noise in the diff (like the moving around of limit_posts for example). Not a huge issue, but something to be aware of, as that can make things harder to review.

All in all, I say :shipit:

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 20, 2015

Member

Thank goodness for @mattr- 🙏 Agreed with all your comments. :)

Member

parkr commented Mar 20, 2015

Thank goodness for @mattr- 🙏 Agreed with all your comments. :)

Made corrections as suggested by @mattr-.
 - Corrected TomDoc, added variables and fixed typos.
 - deprecated_rel_permalink -> relative_permalinks_are_deprecated.
 - Grouped calls together in @reader.read.
 - Removed dynamic and static reader subdirectories.
 - Removed unnecessary move of limit_posts.

Signed-off-by: Martin Jorn Rogalla <martin@martinrogalla.com>
@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 20, 2015

Contributor

Made the corrections as they were suggested by @mattr- in 63a1ec8.

Contributor

MartinRogalla commented Mar 20, 2015

Made the corrections as they were suggested by @mattr- in 63a1ec8.

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Mar 20, 2015

Member

❤️ 💖 💙 💜 💚

:shipit:

Member

mattr- commented Mar 20, 2015

❤️ 💖 💙 💜 💚

:shipit:

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 20, 2015

Member

Looks good! 🆒

Member

alfredxing commented Mar 20, 2015

Looks good! 🆒

@MartinRogalla

This comment has been minimized.

Show comment
Hide comment
@MartinRogalla

MartinRogalla Mar 23, 2015

Contributor

Is there anything I should do on my end, before this can get merged? 😊

Contributor

MartinRogalla commented Mar 23, 2015

Is there anything I should do on my end, before this can get merged? 😊

parkr added a commit that referenced this pull request Mar 24, 2015

@parkr parkr merged commit e7d0b6c into jekyll:master Mar 24, 2015

1 check passed

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

parkr added a commit that referenced this pull request Mar 24, 2015

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 24, 2015

Member

🎉

Member

parkr commented Mar 24, 2015

🎉

@MartinRogalla MartinRogalla deleted the delftswa2014:site_extract_readers branch Mar 24, 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.