Incremental build if destination file missing #3614

Merged
merged 3 commits into from Mar 29, 2015

Conversation

Projects
None yet
5 participants
@nickburlett
Contributor

nickburlett commented Mar 24, 2015

Incremental build doesn't check if the destination file is present (#3591). This means that if the file is accidentally deleted, or the configuration changes what the output file should be, the incremental build will not regenerate the file.

Incrementally regenerate missing destination file
Addresses the third point of #3591, in which the incremental regenerator doesn't notice that destination files have gone missing.
@nickburlett

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Contributor

I mentioned this branch in #3591, but decided a pull request would be a better way to track the requested change.

Contributor

nickburlett commented Mar 24, 2015

I mentioned this branch in #3591, but decided a pull request would be a better way to track the requested change.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

document.asset_file? || document.data['regenerate'] || \
  source_modified_or_dest_missing?(
    site.in_source_dir(document.relative_path), document.destination(@site.dest)
  )

is much more readable. Mind doing it this way? -- There is also no need for the return as it's implicit in Ruby.

document.asset_file? || document.data['regenerate'] || \
  source_modified_or_dest_missing?(
    site.in_source_dir(document.relative_path), document.destination(@site.dest)
  )

is much more readable. Mind doing it this way? -- There is also no need for the return as it's implicit in Ruby.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Owner

Sure, that does seem to be in keeping with the style of the rest of the code. Previously I was trying to keep the lines short, which is a style I'm used to having to follow. Of course, I completely failed to follow that in the else block so I'm not sure what the point was (^_^). On the return statement, I had a slightly different structure for the function in an earlier attempt, and it required short-circuit returns.

Owner

nickburlett replied Mar 24, 2015

Sure, that does seem to be in keeping with the style of the rest of the code. Previously I was trying to keep the lines short, which is a style I'm used to having to follow. Of course, I completely failed to follow that in the else block so I'm not sure what the point was (^_^). On the return statement, I had a slightly different structure for the function in an earlier attempt, and it required short-circuit returns.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

Same comment as above (regarding style and implicit returns.)

Same comment as above (regarding style and implicit returns.)

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

It looks like above that he (reverse) short-circuiting if there is no path, why allow nil path and destination now?

It looks like above that he (reverse) short-circuiting if there is no path, why allow nil path and destination now?

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Owner

My understanding is that Document, Page, and Post always have a .path and .dest. But for other types of objects, source_path needs to be able to be nil for "The site regenerator should always regenerate objects that don't respond to :path". For the destination source_modified_or_dest_missing? returns false if dest_path is nil so that Objects that don't have a destination aren't affected by the new destination check.

Owner

nickburlett replied Mar 24, 2015

My understanding is that Document, Page, and Post always have a .path and .dest. But for other types of objects, source_path needs to be able to be nil for "The site regenerator should always regenerate objects that don't respond to :path". For the destination source_modified_or_dest_missing? returns false if dest_path is nil so that Objects that don't have a destination aren't affected by the new destination check.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

I don't know if the rest of @jekyll/core will accept this but I think this is definately a case of control statements.

((source_path and modified?(source_path)) or true) || (dest_path and !File.exist?(dest_path))

I don't know if the rest of @jekyll/core will accept this but I think this is definately a case of control statements.

((source_path and modified?(source_path)) or true) || (dest_path and !File.exist?(dest_path))

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

To add an extra note you don't need to use "control keywords" here you could just do:

((source_path && modified?(source_path)) or true) || (dest_path && !File.exist?(dest_path))

I would surely leave the or where it is to keep readability though.

To add an extra note you don't need to use "control keywords" here you could just do:

((source_path && modified?(source_path)) or true) || (dest_path && !File.exist?(dest_path))

I would surely leave the or where it is to keep readability though.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Owner

I need to look into this some more. The following fails "The site regenerator should always regenerate objects that don't respond to :path":

    def source_modified_or_dest_missing?(source_path, dest_path)
      #source_modified = source_path ? modified?(source_path)  : true
      source_modified = (source_path and modified?(source_path)) or true
      dest_missing    = (dest_path and !File.exist?(dest_path)) or false
      return source_modified || dest_missing
    end

But if I swap the comments on the source_modified assignment lines, it passes. Clearly I'm not understanding something about Ruby. I'll dig into it tonight.

Owner

nickburlett replied Mar 24, 2015

I need to look into this some more. The following fails "The site regenerator should always regenerate objects that don't respond to :path":

    def source_modified_or_dest_missing?(source_path, dest_path)
      #source_modified = source_path ? modified?(source_path)  : true
      source_modified = (source_path and modified?(source_path)) or true
      dest_missing    = (dest_path and !File.exist?(dest_path)) or false
      return source_modified || dest_missing
    end

But if I swap the comments on the source_modified assignment lines, it passes. Clearly I'm not understanding something about Ruby. I'll dig into it tonight.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 25, 2015

Owner

Ok, I wasn't thinking straight this morning. Two things are wrong with the changed code:

  1. (A and B) or CA ? B : C if B could be false and CB
  2. foo = false or true doesn't parse the way I was expecting:
irb(main):010:0> foo = false or true
=> true
irb(main):011:0> foo
=> false

Instead, I've settled on

      modified?(source_path) || ((dest_path and !File.exist?(dest_path)) or false)

with modified? changed to check for a nil path.

Owner

nickburlett replied Mar 25, 2015

Ok, I wasn't thinking straight this morning. Two things are wrong with the changed code:

  1. (A and B) or CA ? B : C if B could be false and CB
  2. foo = false or true doesn't parse the way I was expecting:
irb(main):010:0> foo = false or true
=> true
irb(main):011:0> foo
=> false

Instead, I've settled on

      modified?(source_path) || ((dest_path and !File.exist?(dest_path)) or false)

with modified? changed to check for a nil path.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 25, 2015

modified?(source_path) || (dest_path && !File.exist?(dest_path))

There really is no need for the (or) because Ruby will evaluate the latter to false and trip the entire expression to false when it falls into the expression but I definitely like your new shorter version!

modified?(source_path) || (dest_path && !File.exist?(dest_path))

There really is no need for the (or) because Ruby will evaluate the latter to false and trip the entire expression to false when it falls into the expression but I definitely like your new shorter version!

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 26, 2015

Owner

Right, but that's not the case I was referring to. Consider when both dest_path and source_path are nil:

[1] pry(main)> def modified?(arg)
[1] pry(main)*   return arg        
[1] pry(main)* end # => :modified?        
=> nil
[2] pry(main)> source_path=nil;
[3] pry(main)> dest_path=nil;
[4] pry(main)> modified?(source_path) || (dest_path && File.exist?(dest_path))
=> nil
Owner

nickburlett replied Mar 26, 2015

Right, but that's not the case I was referring to. Consider when both dest_path and source_path are nil:

[1] pry(main)> def modified?(arg)
[1] pry(main)*   return arg        
[1] pry(main)* end # => :modified?        
=> nil
[2] pry(main)> source_path=nil;
[3] pry(main)> dest_path=nil;
[4] pry(main)> modified?(source_path) || (dest_path && File.exist?(dest_path))
=> nil

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 26, 2015

That matters little to us because truthy is truthy.

That matters little to us because truthy is truthy.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 26, 2015

Owner

Ok, thank you for humoring my concerns. I'm used to coding in environments where precision in typing matters immensely.

Owner

nickburlett replied Mar 26, 2015

Ok, thank you for humoring my concerns. I'm used to coding in environments where precision in typing matters immensely.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 26, 2015

If we were using == or === it would matter a lot to us and precision would matter, but in a case where we simply do !modified? turthy is fine because !nil will evaluate to false. Now precision does matter on an object that does not end with ? because at that point it can be passed around, but this is an internal API where precision matters little since we just wanna know if we need to do it and nil tells us we don't.

If we were using == or === it would matter a lot to us and precision would matter, but in a case where we simply do !modified? turthy is fine because !nil will evaluate to false. Now precision does matter on an object that does not end with ? because at that point it can be passed around, but this is an internal API where precision matters little since we just wanna know if we need to do it and nil tells us we don't.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 26, 2015

Owner

Sounds good. I only brought it up because I didn't feel comfortable changing the returned type in a system that don't have a complete picture of. In other systems I've worked with, functions sometimes return true, false, or nil to indicate yes, no, or unknown.

Thanks again for your help in understanding the jekyll infrastructure. Any thoughts on whether this pull request can be accepted?

Owner

nickburlett replied Mar 26, 2015

Sounds good. I only brought it up because I didn't feel comfortable changing the returned type in a system that don't have a complete picture of. In other systems I've worked with, functions sometimes return true, false, or nil to indicate yes, no, or unknown.

Thanks again for your help in understanding the jekyll infrastructure. Any thoughts on whether this pull request can be accepted?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

This assertion need be explicit. If you created a file test for a file, if you create a folder, test for a file type directory. Implicit testing is bad IMO and this is definitely a fallible test prone to edge abuse.

This assertion need be explicit. If you created a file test for a file, if you create a folder, test for a file type directory. Implicit testing is bad IMO and this is definitely a fallible test prone to edge abuse.

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Owner

I don't understand what you mean. What's an explicit assertion versus an implicit one? I'm happy to fix this, just please help me better understand what you're looking for (^_^)

Also, in my email I received a separate message about adding FIXME comment, but it seems like that message has disappeared. I'm assuming it was related to this line, but I can't tell.

Owner

nickburlett replied Mar 24, 2015

I don't understand what you mean. What's an explicit assertion versus an implicit one? I'm happy to fix this, just please help me better understand what you're looking for (^_^)

Also, in my email I received a separate message about adding FIXME comment, but it seems like that message has disappeared. I'm assuming it was related to this line, but I can't tell.

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 24, 2015

An implicit test is a test that assumes everything is copacetic just because of the existence of an object (file or object in Ruby) where an explicit test takes an adversarial approach and assumes that if I expect an object then it needs to explicitly be this kind of object. The difference is File.exist? vs. File.file? and File.directory?

The FIXME comment was deleted because I decided that I would just note it (or you could) in #3543

An implicit test is a test that assumes everything is copacetic just because of the existence of an object (file or object in Ruby) where an explicit test takes an adversarial approach and assumes that if I expect an object then it needs to explicitly be this kind of object. The difference is File.exist? vs. File.file? and File.directory?

The FIXME comment was deleted because I decided that I would just note it (or you could) in #3543

This comment has been minimized.

Show comment
Hide comment
@nickburlett

nickburlett Mar 24, 2015

Owner

Got it. I'll update the assertion to use File.file?.

Owner

nickburlett replied Mar 24, 2015

Got it. I'll update the assertion to use File.file?.

nickburlett added some commits Mar 25, 2015

Clean up regeneration missing-destination checks
Use easier-to-follow checks for missing-destinations in the regenerator.
Clean up destination modified check
Clean up the destination modified check in `source_modified_or_dest_missing?` to be easier to read. Note that it can now return `nil` instead of `false` for an unmodified `source_path` and a `nil` `dest_path`, but in a discussion on 706007e we decided that was okay.
@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Mar 26, 2015

Contributor

/cc @jekyll/core for review.

Contributor

envygeeks commented Mar 26, 2015

/cc @jekyll/core for review.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Mar 27, 2015

Member

Looks good to me! Thanks @nickburlett

There are still a few slightly complex conditionals, but since the whole file is basically a huge conditional...

Member

alfredxing commented Mar 27, 2015

Looks good to me! Thanks @nickburlett

There are still a few slightly complex conditionals, but since the whole file is basically a huge conditional...

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 29, 2015

Member

Cool, feel free to merge. :shipit:

Member

parkr commented Mar 29, 2015

Cool, feel free to merge. :shipit:

envygeeks added a commit that referenced this pull request Mar 29, 2015

Merge pull request #3614 from nickburlett/patch/incremental-build-des…
…t-missing

Incremental build if destination file missing

@envygeeks envygeeks merged commit f75346c into jekyll:master Mar 29, 2015

1 check passed

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

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