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

Fix --unpublished not affecting collection documents #7027

Merged
merged 3 commits into from May 20, 2018

Conversation

Projects
None yet
6 participants
@philipbelesky
Contributor

philipbelesky commented May 19, 2018

Fixes #7026

Note that the changes to collection.rb different slightly to what was suggested — the fmt linter gave me a warning against using parens with unless. Please let me know if any further modifications are needed.

As a sidenote thanks for the very quick and friendly prompts for how to add this PR. Getting a chance to learn about Cucumber was great!

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 19, 2018

Thanks for opening the PR.
Removing the superfluous parentheses are fine.

I'd like to leave a few comments regarding the included cucumber test:

  • Unpublished documents are never rendered.
    When one passes the --unpublished switch, Jekyll overrides the front matter setting of published: false which leads to all non-future dated documents to be rendered.
  • "Snowy" and "Hardy" are meant to be future-dated like in the preceding scenario. We need this to test how the proposed patch affects such documents.
    (Documents other than posts differ in behavior here. Future-dated documents should be accessible via Liquid, but not written to destination unless --future switch has been passed as well.)
@philipbelesky

This comment has been minimized.

Contributor

philipbelesky commented May 19, 2018

Thanks for the feedback! I have created a second test to account for the scenario when output: false and incorporated a consideration of future-dated documents into both tests. Please let me know if anything else is required (is each case perhaps too extensive in what it is testing?) or you'd prefer the PR closed and re-opened as a single commit.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 19, 2018

The scenarios might look intimidating at first, but it seems just adequate to me.

Do not worry about the commit count. Our bot will squash them when merging..

@ashmaroli ashmaroli requested a review from jekyll/stability May 19, 2018

@ashmaroli ashmaroli added the fix label May 19, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 19, 2018

In case anyone's wondering why the proposed fix doesn't involve the Publisher class here.., this is because it requires "enhancing" Publisher's API to get the desired outcome, which is outside the scope of a "patch" release, as per Principles of SemVer..

This will be handled by updating #6997 for v4.0

Then I should get a zero exit status
And the _site directory should exist
And I should see "<div>Rover</div>" in "_site/index.html"
But I should see "<div>Snowy</div>" in "_site/index.html"

This comment has been minimized.

@DirtyF

DirtyF May 19, 2018

Member

A document with a future date gets published even without --future option?

This comment has been minimized.

@pathawks

pathawks May 19, 2018

Member

I don't think it gets output, but it does get rendered and is accessible via Liquid. It's kind of weird behavior. 🤷‍♂️

@DirtyF

This comment has been minimized.

Member

DirtyF commented May 19, 2018

Future-dated documents should be accessible via Liquid

Why is that?

@ashmaroli ashmaroli referenced this pull request May 19, 2018

Merged

Release 💎 3.8.2 #7023

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented May 19, 2018

Future-dated documents should be accessible via Liquid

Why is that?

@DirtyF For example, one might have a collection labelled "events" with documents for past, current and upcoming events which are then rendered using Liquid for loop..

Prior discussion: #6506 (comment)

@@ -212,7 +212,9 @@ def container
def read_document(full_path)
doc = Document.new(full_path, :site => site, :collection => self)
doc.read
docs << doc unless doc.data["published"] == false
unless doc.data["published"] == false && !site.unpublished

This comment has been minimized.

@pathawks

pathawks May 19, 2018

Member

This unless makes my head hurt.

"Unless not the site is unpublished"

This comment has been minimized.

@pathawks

pathawks May 19, 2018

Member
if site.unpublished || doc.data["published"] != false
  docs << doc
end

This comment has been minimized.

@ashmaroli

ashmaroli May 19, 2018

Member

Reversing the conditional leads us to another cleaner alternative:

if site.unpublished || doc.published?
  docs << doc
end

This comment has been minimized.

@philipbelesky

philipbelesky May 20, 2018

Contributor

Agreed, just pushed up a commit that amends this.

@pathawks pathawks added this to the 3.8.3 milestone May 19, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented May 19, 2018

Wow, great work here! I appreciate the tests that have been added 🎉

@DirtyF

DirtyF approved these changes May 20, 2018

❤️ Thanks @philipbelesky

@ashmaroli

LGTM! 👍

@oe

oe approved these changes May 20, 2018

LGTM!

@pathawks

This comment has been minimized.

Member

pathawks commented May 20, 2018

@jekyllbot: :shipit: +fix

@jekyllbot jekyllbot merged commit 9aec161 into jekyll:master May 20, 2018

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

@jekyllbot jekyllbot added bug fix labels May 20, 2018

pathawks added a commit that referenced this pull request May 20, 2018

pathawks added a commit that referenced this pull request May 20, 2018

@pathawks pathawks referenced this pull request May 20, 2018

Merged

Backport #7027 #7028

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