Skip to content
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

Implement postprocessing #726

Merged
merged 18 commits into from Nov 29, 2015

Conversation

Projects
None yet
2 participants
@gjtorikian
Copy link
Member

gjtorikian commented Nov 12, 2015

This is a start to the implementation of a postprocessor block. I'm opening it up somewhat early to get some feedback on whether I'm on the right track.

I mostly copied the work going on for the preprocessor block. I'd like to have an immutable items collection, with the addition of methods like updated? and added?. That's what attributed_item_collection_view.rb is for.

Closes #711.

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 12, 2015

Looks good so far!

I think there’s potential for quite a bit of refactoring here (e.g. unify code for pre- and postprocessor, move tests into specs) but that can wait.

@gjtorikian gjtorikian self-assigned this Nov 16, 2015

@gjtorikian

This comment has been minimized.

Copy link
Member Author

gjtorikian commented Nov 23, 2015

I can't seem to figure out how to get access to whether an item was created or modified. The only place I see such information use is right after a rep is written and notified. What would be the best way to store this information so it remains in the postprocess step? Should each item handle its own state via a new attribute?

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 23, 2015

Hmm, that’s annoying.

It seems like a good idea to have #created? and #modified? on an item rep view (at least in the post-processor). I’m OK with storing that information on an item (or item rep) for now, but I imagine this might be refactored later on (and the concept of views should make this easy).

@gjtorikian

This comment has been minimized.

Copy link
Member Author

gjtorikian commented Nov 26, 2015

I feel like I'm pretty close to getting #created? and #modified? working. Well, I mean, these methods now exist and they do something, but something feels very dirty and wrong. I like the idea of setting the item.rep status after the compilation, but I'm not sure I've done it correctly. Unfortunately I'm going to have to tap out of continuing this work—I'm running up against the limitations of my knowledge of nanoc internals here. 😅

On the bright side, it does seem like postprocessing as a whole is working.

@gjtorikian gjtorikian removed their assignment Nov 26, 2015

end

def updated?
reps.select { |rep| rep.status == :modified }

This comment has been minimized.

@ddfreyne

ddfreyne Nov 27, 2015

Member

Can you replace :modified with :updated? This way, it reflects the name of the method.

This comment has been minimized.

@ddfreyne

ddfreyne Nov 27, 2015

Member

See other (big) comment—these two methods might not be necessary.

@@ -233,8 +237,10 @@ def compile_rep(rep)
if can_reuse_content_for_rep?(rep)
Nanoc::Int::NotificationCenter.post(:cached_content_used, rep)
rep.snapshot_contents = compiled_content_cache[rep]
rep.status = :created

This comment has been minimized.

@ddfreyne

ddfreyne Nov 27, 2015

Member

Hmm, this :created is incorrect. Some context:

If #can_reuse_content_for_rep? returns true, then that means that the compiled content (which was cached in the previous compilation run) can be reused without actually recompiling the item. This is used when an outdated item includes content from an item that is not outdated, and whose content can thus be reused without recompiling.

In the ItemRepWriter, Nanoc determines whether the resulting file exists already (is_created), is modified (is_modified), or neither. (See these ItemRepWriter lines.)

It’d be useful to make use of the ItemRepWriter modified/created logic that already exists. Creating a snapshot for which a route is defined will make the item rep writer write out the compiled content. The :last snapshot, containing the compiled content after the entire rule is executed, is trigged by the line executor.snapshot(rep, :last) in the #recalculate_content_for_rep method below.

Perhaps the ItemRepWriter could update the item rep to set item_rep.modified = is_modified, with is_modified being whatever ItemRepWriter already calculates. (I don’t think there’s a need for a similar created attribute.) The advantage of this is that only ItemRepWriter determines whether content is deemed changed. In the following cases, ItemRepWriter will treat the content as not modified:

  • An item that is recompiled (#can_reuse_content_for_rep? returns false), but whose content turns out to be identical after compilation
  • An item whose content is reused (#can_reuse_content_for_rep? returns true)
  • An item that’s not even considered for compilation (not considered outdated)
@@ -0,0 +1,11 @@
module Nanoc
class PostCompileItemView < Nanoc::ItemView

This comment has been minimized.

@ddfreyne

ddfreyne Nov 27, 2015

Member

👍 for having PostCompileItemView. I got a little lazy and didn’t create a PreprocessItemView (and friends, like the collection view), but that’ll change Soon™. (See #571.)

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 27, 2015

This is looking quite good so far. Making use of the ItemRepWriter is the only change I’d like to see, but other than that, it’s mergeable!

@gjtorikian

This comment has been minimized.

Copy link
Member Author

gjtorikian commented Nov 29, 2015

Ah, thanks for the breakdown. I think the naming of the class was confusing me, without doing any inspection. I believe the class is actually called when the rep is going to be written to disk. It didn't occur to me that its created/modified checks could still be useful.

I took nearly all of your suggestions. I kept the modified method on PostCompileItemView, though, since it seems like a useful helper to have.

@@ -23,6 +23,9 @@ class ItemRep
# @return [Enumerable<Nanoc::Int:SnapshotDef]
attr_accessor :snapshot_defs

# @return [Boolean]
attr_accessor :modified

This comment has been minimized.

@ddfreyne

ddfreyne Nov 29, 2015

Member

Add alias_method :compiled?, :compiled for extra sweetness

This comment has been minimized.

@gjtorikian

gjtorikian Nov 29, 2015

Author Member

👍 I like sweetness.

@@ -27,6 +27,11 @@ def name
@item_rep.name
end

# @return [Boolean]

This comment has been minimized.

@ddfreyne

ddfreyne Nov 29, 2015

Member

I like modified (more than updated which I use elsewhere) but I’m still hesitant to expose this as part of the public API just yet. I’d prefer to have it just on PostCompileItemView for the time being.

(I’ve been bitten by trying to expose too much before, so I’d like to avoid that!)

This comment has been minimized.

@ddfreyne

ddfreyne Nov 29, 2015

Member

In other words, could you add # @api private so it’s marked as private in the API docs?

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 29, 2015

👍 otherwise—looks awesome!

@gjtorikian

This comment has been minimized.

Copy link
Member Author

gjtorikian commented Nov 29, 2015

Fair enough—it's your project! 🎆

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 29, 2015

Sweet! Will merge as soon as Travis CI gives the green light.

@ddfreyne ddfreyne changed the title [WIP] Implementation of postprocessing Implemeting postprocessing Nov 29, 2015

@ddfreyne ddfreyne added this to the 4.1.0 milestone Nov 29, 2015

@ddfreyne ddfreyne changed the title Implemeting postprocessing Implement postprocessing Nov 29, 2015

ddfreyne added a commit that referenced this pull request Nov 29, 2015

Merge pull request #726 from nanoc/postprocess-life
Implement postprocessing

@ddfreyne ddfreyne merged commit bebc31c into master Nov 29, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.08%) to 91.677%
Details

@ddfreyne ddfreyne deleted the postprocess-life branch Nov 29, 2015

@ddfreyne

This comment has been minimized.

Copy link
Member

ddfreyne commented Nov 29, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.