Add PageWithoutAFile class from jekyll plugins #6556

ashmaroli · 2017-11-14T04:31:56Z

Official plugins jekyll-sitemap, jekyll-feed and a couple more add a Page subclass to generate a file (usually from Liquid templates not present in the user's source directory).

Advantages

Users adding both plugins need not allocate memory for the twin subclasses.
Also allows other third-party plugins that follows the above implementation.

Plugins known to implement this subclass or similar:

jekyll-feed and jekyll-sitemap add a Page subclass to generate a file from liquid templates not present in the user's source directory. Advantages - Users adding both plugins need not allocate memory for the twin subclasses. - Also allows other third-party plugins that follows the above implementation.

pathawks · 2017-11-14T05:35:16Z

I like this idea.
I’m not sure if this is the best place to put this subclass, but it is so small that I’m ok with it.

pathawks · 2017-11-14T05:56:26Z

Plugins wouldn’t be able to take advantage of this right away, as we tend to support older versions of Jekyll.

I wonder if, as long as we’re consolidating this, we could provide a mechanism to let the user know where a PageWithoutAFile originated, since the path is useless.

ashmaroli · 2017-11-14T06:18:34Z

take advantage of this right away, as we tend to support older versions of Jekyll.

I understand.. but it'll be easier once GitHub Pages switches onto a release with this patch.

I wonder if, as long as we’re consolidating this, we could provide a mechanism to let the user know where a PageWithoutAFile originated, since the path is useless.

A log message would be great. But, its better if its implemented by the concerned plugin itself..

           JekyllFeed: feed.xml generated
        JekyllSitemap: sitemap.xml generated

ashmaroli · 2017-11-14T06:24:15Z

@benbalter I'd like your thoughts on this.. Thanks.

mattr- · 2017-11-14T14:41:41Z

lib/jekyll/page.rb

@@ -184,4 +184,14 @@ def write?
      true
    end
  end
+
+  class PageWithoutAFile < Page


I would prefer a different name such as BasePage or BasicPage here. The fact that it doesn't have a file is irrelevant, IMHO

Naming has been a rough sea here at Jekyll AFAIR.. so I'll change it to whatever the majority zeros in..

mattr- · 2017-11-14T14:42:14Z

lib/jekyll/page.rb

@@ -184,4 +184,14 @@ def write?
      true
    end
  end
+
+  class PageWithoutAFile < Page
+    # ---------------------------------------------------------------


This documentation here isn't enough for justifying and explaining the existence of this class. Could we get some proper class level documentation?

mattr- · 2017-11-14T14:43:46Z

Users adding both plugins need not allocate memory for the twin subclasses.

Do you have hard numbers for this?

I do like this idea. Something that multiple plugins are using should be in the core. With just a few small tweaks, I think this will be ready.

ashmaroli · 2017-11-14T14:44:35Z

Do you have hard numbers for this?

Sorry, no. just an assumption..

jekyllbot

~~I'm alive~~

parkr

I'm 👍 on this! Can we put it in its own file instead of putting it in page.rb? We'll also want a few tests to help define the API boundaries for adding content and verify that it doesn't access any files 😄

Crunch09

LGTM, just two small comments 👍

Crunch09 · 2017-11-29T23:47:10Z

test/test_page_without_a_file.rb

+      }))
+    end
+
+    context "on intialized" do


on intialize ? Or maybe even with default config ? What do you think?

~~I disagree with with default config.. PageWithoutAFile instances do not change on passing different parameters. For a given site, its always nil content and empty data hash.~~

Crunch09 · 2017-11-29T23:57:18Z

test/test_page_without_a_file.rb

+require "helper"
+
+class TestPageWithoutAFile < JekyllUnitTest
+  def setup_page(*args)


tiny thing: Instead of overriding this entire method to have the only difference in having PageWithoutAFile.new instead of Page.new ,what do you think of changing the method signature to something like:

def setup_page(*args, klass: Page)

Thank you for the suggestion. I had considered using a similar signature (klass=Page, *args), but opted out since I'm creating a Jekyll::Page instance only once in this new unit-test-class.

That said, I like your syntax with Keyword Arguments. Improves future tests-scalability. 👍

@Crunch09

props to @Crunch09 for the suggestion

Crunch09 · 2017-11-30T13:48:42Z

test/test_page_without_a_file.rb

@@ -3,13 +3,13 @@
 require "helper"

 class TestPageWithoutAFile < JekyllUnitTest
-  def setup_page(*args)
+  def setup_page(*args, base: source_dir, klass: PageWithoutAFile)


@ashmaroli Hey, sorry my first suggestion was unclear: I tought about moving setup_page, which is also defined in TestPage, to JekyllUnitTest and give it then a klass option because that's the only difference between those two methods and then we would only define it once :)

I see.. I should've understood your obvious intention.. my bad..

No worries at all! It's already better 😉

I don't want to change test_page.rb in this PR (since its an addition-only patch, right now..). Perhaps, a separate future PR would be best..

Fair enough 👍

DirtyF · 2017-11-30T18:16:35Z

@jekyllbot: merge +dev

Merge pull request 6556

pathawks approved these changes Nov 14, 2017

View reviewed changes

ashmaroli requested a review from benbalter November 14, 2017 06:23

mattr- requested changes Nov 14, 2017

View reviewed changes

proper class level documentation

4718484

jekyllbot reviewed Nov 27, 2017

View reviewed changes

parkr reviewed Nov 27, 2017

View reviewed changes

ashmaroli changed the title ~~Add PageWithoutAFile class from jekyll plugins~~ Add PageWithoutAFile class from jekyll plugins [WIP] Nov 28, 2017

ashmaroli added 2 commits November 28, 2017 23:44

move Jekyll::PageWithoutAFile to a separate file

bd871ae

add tests for PageWithoutAFile class

1b832bc

ashmaroli changed the title ~~Add PageWithoutAFile class from jekyll plugins [WIP]~~ Add PageWithoutAFile class from jekyll plugins Nov 28, 2017

parkr approved these changes Nov 28, 2017

View reviewed changes

ashmaroli added this to the v3.7.0 milestone Nov 29, 2017

Crunch09 approved these changes Nov 29, 2017

View reviewed changes

ashmaroli added 2 commits November 30, 2017 11:22

improvise with keyword arguments

067fdfd

props to @Crunch09 for the suggestion

add proper test for processing and writing to dest

02d8e9c

ashmaroli force-pushed the page-without-file branch from 4880329 to 02d8e9c Compare November 30, 2017 07:44

return proper debug string while inspecting class

d0905ab

Crunch09 reviewed Nov 30, 2017

View reviewed changes

jekyllbot merged commit 3834200 into jekyll:master Nov 30, 2017

jekyllbot added fix internal labels Nov 30, 2017

jekyllbot added a commit that referenced this pull request Nov 30, 2017

Update history to reflect merge of #6556 [ci skip]

5a10887

DirtyF removed the request for review from benbalter November 30, 2017 18:17

ashmaroli deleted the page-without-file branch December 1, 2017 07:30

DirtyF pushed a commit that referenced this pull request Dec 7, 2017

Add PageWithoutAFile class from jekyll plugins (#6556)

65f7dec

Merge pull request 6556

DirtyF pushed a commit that referenced this pull request Dec 7, 2017

Update history to reflect merge of #6556 [ci skip]

0c4b12e

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PageWithoutAFile class from jekyll plugins #6556

Add PageWithoutAFile class from jekyll plugins #6556

ashmaroli commented Nov 14, 2017 •

edited

Loading

pathawks commented Nov 14, 2017

pathawks commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

mattr- Nov 14, 2017

ashmaroli Nov 14, 2017

mattr- Nov 14, 2017

mattr- commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

jekyllbot left a comment •

edited

Loading

parkr left a comment

Crunch09 left a comment

Crunch09 Nov 29, 2017

ashmaroli Nov 30, 2017 •

edited

Loading

Crunch09 Nov 29, 2017

ashmaroli Nov 30, 2017

Crunch09 Nov 30, 2017

ashmaroli Nov 30, 2017

Crunch09 Nov 30, 2017

ashmaroli Nov 30, 2017

Crunch09 Nov 30, 2017

DirtyF commented Nov 30, 2017

Add PageWithoutAFile class from jekyll plugins #6556

Add PageWithoutAFile class from jekyll plugins #6556

Conversation

ashmaroli commented Nov 14, 2017 • edited Loading

pathawks commented Nov 14, 2017

pathawks commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattr- commented Nov 14, 2017

ashmaroli commented Nov 14, 2017

jekyllbot left a comment • edited Loading

Choose a reason for hiding this comment

parkr left a comment

Choose a reason for hiding this comment

Crunch09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashmaroli Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirtyF commented Nov 30, 2017

ashmaroli commented Nov 14, 2017 •

edited

Loading

jekyllbot left a comment •

edited

Loading

ashmaroli Nov 30, 2017 •

edited

Loading