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

Adding a debug log statment for skipped future documents. #4558

Merged
merged 1 commit into from Mar 8, 2016

Conversation

Projects
None yet
5 participants
@zandaleph
Contributor

zandaleph commented Feb 21, 2016

For #4507

All tests pass, but didn't add any new tests for the log line. There's only one test that looks at log output at this point, and I didn't know enough to write a good test of this change.

One thing I wasn't sure was okay - this change places a requirement on all things to be published to have a #relative_path method. I'm not sure how to validate if this is a good assumption or not. Guidance here would be appreciated.

Sample output of jekyll build --verbose:

   Logging at level: debug
 Configuration file: /Users/zspencer/Code/testblog/_config.yml
          Requiring: kramdown
             Source: /Users/zspencer/Code/testblog
        Destination: /Users/zspencer/Code/testblog/_site
  Incremental build: disabled. Enable with --incremental
       Generating...
            Reading: _posts/2016-02-10-welcome-to-jekyll.markdown
            Reading: _posts/2016-02-29-using-jekyll-as-a-layout-engine.markdown
           Skipping: _posts/2016-02-29-using-jekyll-as-a-layout-engine.markdown because it is in the future.
          Rendering: _posts/2016-02-10-welcome-to-jekyll.markdown
   Pre-Render Hooks: _posts/2016-02-10-welcome-to-jekyll.markdown
   Rendering Liquid: _posts/2016-02-10-welcome-to-jekyll.markdown
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 21, 2016

Contributor

I like the premise, I don't like the spot it's place in though, boolean methods shouldn't be logging like that because they can be called dozens of times to validate something which can lead to plenty of duplicate logging. Is there no better place to put it?

/cc @jekyll/core -- @parkr maybe you can point out a better place?

Contributor

envygeeks commented Feb 21, 2016

I like the premise, I don't like the spot it's place in though, boolean methods shouldn't be logging like that because they can be called dozens of times to validate something which can lead to plenty of duplicate logging. Is there no better place to put it?

/cc @jekyll/core -- @parkr maybe you can point out a better place?

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Feb 21, 2016

Contributor

I'm not a huge fan of the location, but as the code is currently factored there's nowhere else this statement can go without either duplicating the logic in Publisher#hidden_in_the_future? or refactoring (which I am not familiar enough with the jekyll project to do without guidance). The method is only called as part of the Publisher#publish? method, and since that method considers factors other than the post being in the future, there's no way to tell from the result of that method why the thing should not be published.

I think it is a reasonable location because while it is possible that it could be called dozens of times on the same object, currenly Publisher#publish? is only used in an Enumerable#select context, which means that once an object is found to be unpublishable, it is not considered again. I think it is reasonable to expect that this will continue to be the only way Publisher#publish? will be used until a refactoring is necessary for other reasons.

Contributor

zandaleph commented Feb 21, 2016

I'm not a huge fan of the location, but as the code is currently factored there's nowhere else this statement can go without either duplicating the logic in Publisher#hidden_in_the_future? or refactoring (which I am not familiar enough with the jekyll project to do without guidance). The method is only called as part of the Publisher#publish? method, and since that method considers factors other than the post being in the future, there's no way to tell from the result of that method why the thing should not be published.

I think it is a reasonable location because while it is possible that it could be called dozens of times on the same object, currenly Publisher#publish? is only used in an Enumerable#select context, which means that once an object is found to be unpublishable, it is not considered again. I think it is reasonable to expect that this will continue to be the only way Publisher#publish? will be used until a refactoring is necessary for other reasons.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Feb 22, 2016

Contributor

Well it's not a matter of whether we use it only once, it's a matter of who else uses it more than once since that's a public API. If there is no better place to put it and it's really only used once then there is certainly room for refactoring but I'll digress on when that happens to other @jekyll/core.

Contributor

envygeeks commented Feb 22, 2016

Well it's not a matter of whether we use it only once, it's a matter of who else uses it more than once since that's a public API. If there is no better place to put it and it's really only used once then there is certainly room for refactoring but I'll digress on when that happens to other @jekyll/core.

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Feb 22, 2016

Contributor

Agh, of course! I hadn't considered other consumers of Publisher outside of the core Jekyll code. I feel a bit silly now.

Alright, since this proposed solution is even worse than I had realized, I'd argue that the only way to know why a document wasn't published is to have an alternative method on Publisher (say, #check_publish) which returns a hash of {:should_publish => boolean, :reason => string}. Then #publish? can just delegate to #check_publish and return the first item, and integrators which need more visibility into why an item couldn't be published can call #check_publish instead. Does this sound like a reasonable way forward? If so I think I can hack something together like this in my spare time over the next couple of days.

Contributor

zandaleph commented Feb 22, 2016

Agh, of course! I hadn't considered other consumers of Publisher outside of the core Jekyll code. I feel a bit silly now.

Alright, since this proposed solution is even worse than I had realized, I'd argue that the only way to know why a document wasn't published is to have an alternative method on Publisher (say, #check_publish) which returns a hash of {:should_publish => boolean, :reason => string}. Then #publish? can just delegate to #check_publish and return the first item, and integrators which need more visibility into why an item couldn't be published can call #check_publish instead. Does this sound like a reasonable way forward? If so I think I can hack something together like this in my spare time over the next couple of days.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Feb 22, 2016

Member

@zandaleph I don't see why that is necessary. Why not add it where the publisher.publish? call is actually made?

Member

parkr commented Feb 22, 2016

@zandaleph I don't see why that is necessary. Why not add it where the publisher.publish? call is actually made?

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Feb 24, 2016

Contributor

@parkr Okay, I've done the change as you suggested - only doing the check where the Publisher.publish? call is actually made. There are two things I don't like about this change:

  1. I've had to expose another public method on Publisher.
  2. I've had to duplicate logic from publish? in order to disambiguate why the post was skipped.

If this is still the preferred approach, there it is.

I've left alone the logic from PageReader and Collection where publish? is called, the former because pages aren't dated (I assume), and the latter because it already has logging on skipping, albeit without explaining why.

Contributor

zandaleph commented Feb 24, 2016

@parkr Okay, I've done the change as you suggested - only doing the check where the Publisher.publish? call is actually made. There are two things I don't like about this change:

  1. I've had to expose another public method on Publisher.
  2. I've had to duplicate logic from publish? in order to disambiguate why the post was skipped.

If this is still the preferred approach, there it is.

I've left alone the logic from PageReader and Collection where publish? is called, the former because pages aren't dated (I assume), and the latter because it already has logging on skipping, albeit without explaining why.

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Feb 25, 2016

Contributor

@jekyll/core Any further work that needs done on this change? I'm still happy to prototype a more comprehensive solution for signaling why publish? decided to skip something if needed.

Contributor

zandaleph commented Feb 25, 2016

@jekyll/core Any further work that needs done on this change? I'm still happy to prototype a more comprehensive solution for signaling why publish? decided to skip something if needed.

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Mar 7, 2016

Contributor

@parkr @envygeeks Not sure what the etiquette is on bumping pull requests, so this is the last time I'll update this issue unless I get some feedback. I believe this change captures what I was looking for in #4507 . Please let me know if there's anything I can do.

Contributor

zandaleph commented Mar 7, 2016

@parkr @envygeeks Not sure what the etiquette is on bumping pull requests, so this is the last time I'll update this issue unless I get some feedback. I believe this change captures what I was looking for in #4507 . Please let me know if there's anything I can do.

Show outdated Hide outdated lib/jekyll/readers/post_reader.rb
if !publish && site.publisher.hidden_in_the_future?(doc)
Jekyll.logger.debug "Skipping:", "#{doc.relative_path} because it is in the future."
end
publish

This comment has been minimized.

@parkr

parkr Mar 7, 2016

Member

I like it! Let's change up the style though, using #tap:

site.publisher.publish?(doc).tap do |will_publish|
  if !will_publish && site.publisher.hidden_in_the_future?(doc)
    Jekyll.logger.debug "Skipping:", "#{doc.relative_path} has a future date"
  end
end
@parkr

parkr Mar 7, 2016

Member

I like it! Let's change up the style though, using #tap:

site.publisher.publish?(doc).tap do |will_publish|
  if !will_publish && site.publisher.hidden_in_the_future?(doc)
    Jekyll.logger.debug "Skipping:", "#{doc.relative_path} has a future date"
  end
end

This comment has been minimized.

@zandaleph

zandaleph Mar 7, 2016

Contributor

I had no idea you could #tap arbitrary values - I've only ever used it on Enumerables before. I like it, and I've updated the change to use your suggestion. Thanks!

@zandaleph

zandaleph Mar 7, 2016

Contributor

I had no idea you could #tap arbitrary values - I've only ever used it on Enumerables before. I like it, and I've updated the change to use your suggestion. Thanks!

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 8, 2016

Contributor

I'm 👍 now, seems like this also addresses your concerns too @parkr?

Contributor

envygeeks commented Mar 8, 2016

I'm 👍 now, seems like this also addresses your concerns too @parkr?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 8, 2016

Member

I'm 👍 now, seems like this also addresses your concerns too @parkr?

Indeed, it does!

Thanks for your patience, @zandaleph! And thanks for the pull request. 😄

@jekyllbot: merge +minor

Member

parkr commented Mar 8, 2016

I'm 👍 now, seems like this also addresses your concerns too @parkr?

Indeed, it does!

Thanks for your patience, @zandaleph! And thanks for the pull request. 😄

@jekyllbot: merge +minor

jekyllbot added a commit that referenced this pull request Mar 8, 2016

@jekyllbot jekyllbot merged commit 85685a8 into jekyll:master Mar 8, 2016

1 check passed

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

@parkr parkr added this to the 3.2 milestone Mar 8, 2016

jekyllbot added a commit that referenced this pull request Mar 8, 2016

@zandaleph zandaleph deleted the zandaleph:log-future branch Mar 8, 2016

@zandaleph

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Mar 8, 2016

Contributor

My pleasure, thank you for the excellent feedback and guidance.

Contributor

zandaleph commented Mar 8, 2016

My pleasure, thank you for the excellent feedback and guidance.

@ravicious

This comment has been minimized.

Show comment
Hide comment
@ravicious

ravicious Oct 9, 2016

Thank you for this commit! ❤️❤️❤️

I'm on 3.1.0 and I just lost half an hour on figuring out why my post won't show up. I figured out that by default Jekyll hides posts with a future date. Then I was confused as to why it doesn't say anything about it in the logs just to find out this has been fixed in a new version released three days ago. 😉

ravicious commented on 1391248 Oct 9, 2016

Thank you for this commit! ❤️❤️❤️

I'm on 3.1.0 and I just lost half an hour on figuring out why my post won't show up. I figured out that by default Jekyll hides posts with a future date. Then I was confused as to why it doesn't say anything about it in the logs just to find out this has been fixed in a new version released three days ago. 😉

This comment has been minimized.

Show comment
Hide comment
@zandaleph

zandaleph Oct 10, 2016

Contributor
Contributor

zandaleph replied Oct 10, 2016

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