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

Add PageWithoutAFile class from jekyll plugins #6556

Merged
merged 7 commits into from Nov 30, 2017

Conversation

Projects
None yet
7 participants
@ashmaroli
Member

ashmaroli commented Nov 14, 2017

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:

add PageWithoutAFile class from jekyll plugins
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

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 14, 2017

Member

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.

Member

pathawks commented Nov 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 14, 2017

Member

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.

Member

pathawks commented Nov 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 14, 2017

Member

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
Member

ashmaroli commented Nov 14, 2017

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 ashmaroli requested a review from benbalter Nov 14, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 14, 2017

Member

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

Member

ashmaroli commented Nov 14, 2017

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

Show outdated Hide outdated lib/jekyll/page.rb Outdated
Show outdated Hide outdated lib/jekyll/page.rb Outdated
@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Nov 14, 2017

Member

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.

Member

mattr- commented Nov 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 14, 2017

Member

Do you have hard numbers for this?

Sorry, no. just an assumption..

Member

ashmaroli commented Nov 14, 2017

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 😄

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

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

@parkr

parkr approved these changes Nov 28, 2017

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

@Crunch09

LGTM, just two small comments 👍

Show outdated Hide outdated test/test_page_without_a_file.rb Outdated
Show outdated Hide outdated test/test_page_without_a_file.rb Outdated
@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 30, 2017

Member

@jekyllbot: merge +dev

Member

DirtyF commented Nov 30, 2017

@jekyllbot: merge +dev

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

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@DirtyF DirtyF removed the request for review from benbalter Nov 30, 2017

@ashmaroli ashmaroli deleted the ashmaroli:page-without-file branch Dec 1, 2017

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

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

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