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

Revise and improve publishing logic #1492

Merged
merged 7 commits into from Apr 15, 2014

Conversation

Projects
None yet
4 participants
@maul-esel
Contributor

maul-esel commented Sep 2, 2013

This PR refactors the publishing logic by moving it out to a Publisher class. It also adds the possibility to mark a page as published: false, as already possible for posts (thus this supplements and closes #1080), and a new --unpublished flag to render such posts and pages.

end
def publish?(thing)
can_be_published?(thing) && !hidden_in_the_future?(thing)

This comment has been minimized.

@maul-esel

maul-esel Sep 2, 2013

Contributor

Awful naming for both, I know. Couldn't come up with any idea though. Can you?

This comment has been minimized.

@mattr-

mattr- Sep 3, 2013

Member

There's nothing wrong with the naming here. 😃

end
def hidden_in_the_future?(thing)
thing.is_a?(Post) && !@site.future && thing.date > @site.time

This comment has been minimized.

@maul-esel

maul-esel Sep 2, 2013

Contributor

I thought it would make little sense for pages to be hidden based on some date. Do you agree?

This comment has been minimized.

@mattr-

mattr- Sep 3, 2013

Member

Agreed.

@@ -359,12 +359,12 @@ def do_render(post)
context "initializing posts" do
should "publish when published yaml is no specified" do
post = setup_post("2008-02-02-published.textile")
assert_equal true, post.published
assert_equal true, @site.send(:publisher).publish?(post)

This comment has been minimized.

@mattr-

mattr- Sep 3, 2013

Member

Why not just use Jekyll::Publisher directly here?

This comment has been minimized.

@maul-esel

maul-esel Sep 3, 2013

Contributor

Valid point, I'll change it.

This comment has been minimized.

@maul-esel

maul-esel Sep 3, 2013

Contributor

Should I still let it in test_post.rb then? And with the cucumber feature, is it even still necessary?

This comment has been minimized.

@maul-esel

maul-esel Sep 10, 2013

Contributor

@mattr- unless you think these tests are still necessary, I'll just remove them.

This comment has been minimized.

@mattr-

mattr- Sep 10, 2013

Member

if it's covered somewhere else, then I think these tests can be removed.

This comment has been minimized.

@maul-esel

maul-esel Sep 10, 2013

Contributor

Done!

@mattr-

This comment has been minimized.

Member

mattr- commented Sep 3, 2013

Seems ok, although I don't understand the difference between --unpublished and --drafts in most cases.

@parkr

This comment has been minimized.

Member

parkr commented Sep 3, 2013

@mattr- --unpublished is for posts and --drafts is for drafts. 😉

@mattr-

This comment has been minimized.

Member

mattr- commented Sep 3, 2013

i.e. for future dated posts?

@parkr

This comment has been minimized.

Member

parkr commented Sep 3, 2013

@mattr- Correct! :)

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 3, 2013

@mattr-, @parkr: Incorrect!

  • --drafts is for drafts, i.e. files in the _drafts folder.
  • --future is for future dated posts
  • --unpublished will be for posts with published: false in their frontmatter
@mattr-

This comment has been minimized.

Member

mattr- commented Sep 3, 2013

Wow, three flags for this? Seems like overkill. Is there a way we can
improve the situation?

On Tue, Sep 3, 2013 at 1:03 PM, maul-esel notifications@github.com wrote:

@mattr- https://github.com/mattr-, @parkr https://github.com/parkr:
Incorrect!

  • --drafts is for drafts, i.e. files in the _drafts folder.
  • --future is for future dated posts
  • --unpublished will be for posts with published: false in their
    frontmatter


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-23733789
.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 3, 2013

Well, three flags for three similar yet different features. Can't really drop the features or the two existing flags because of incompatibility, so the only possible (though probably not recommendable) thing would be some option --render_all that combines them. I guess the request for a flag only to render unpublished posts would pop up again soon though. 😉

So it seems to me there's no good way to avoid having three flags.

@mattr-

This comment has been minimized.

Member

mattr- commented Sep 3, 2013

Something to keep in mind for Jekyll 2.0. 😃

On Tue, Sep 3, 2013 at 1:23 PM, maul-esel notifications@github.com wrote:

Well, three flags for three similar yet different features. Can't really
drop the features or the two existing flags because of incompatibility, so
the only possible (though probably not recommendable) thing would be
some option --render_all that combines them. I guess the request for a
flag only to render unpublished posts would pop up again soon though. [image:
😉]

So it seems to me there's no good way to avoid having three flags.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1492#issuecomment-23735276
.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 12, 2013

Should be ready to be 🚢ed.

@parkr

This comment has been minimized.

Member

parkr commented Sep 15, 2013

Re-reading the code, I think there's still a bunch of entanglement. What if we just passed metadata or something to a class method in a #published? method? Not suuuuure about this yet.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Oct 24, 2013

@parkr: OK, so I re-did this, trying to do what you said, hopefully understanding you correct. Could you have a look at this and if that's basically what you meant, I'll open a new PR for this?

Then the _site directory should exist
And the "_site/index.html" file should exist
And the "_site/public.html" file should exist
And the "_site/secret.html" file should exist

This comment has been minimized.

@parkr

parkr Oct 24, 2013

Member

The published flag is also valid for posts. Could you please add a similar example with posts instead of pages?

@parkr

This comment has been minimized.

Member

parkr commented Oct 24, 2013

Looks like there is a merge conflict here with current master, too.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Oct 24, 2013

Ok, I think you might have missed what I meant. My mistake though, that link in my previous comment is basically invisible. So the re-done stuff is here: maul-esel/jekyll@master...publisher.

@parkr

This comment has been minimized.

Member

parkr commented Apr 7, 2014

This needs a rebase. Would you mind, @maul-esel?

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Apr 7, 2014

@parkr: No problem, done!

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Apr 7, 2014

Oops, sorry, my master branch was out of date. Now it should be fine.

mattr- added a commit that referenced this pull request Apr 15, 2014

@mattr- mattr- merged commit 6be33cf into jekyll:master Apr 15, 2014

mattr- added a commit that referenced this pull request Apr 15, 2014

@maul-esel maul-esel deleted the maul-esel:publishing branch Apr 15, 2014

@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.