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
Merged

Implement postprocessing #726

merged 18 commits into from Nov 29, 2015

Conversation

gjtorikian
Copy link
Contributor

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.

@denisdefreyne
Copy link
Member

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
Copy link
Contributor Author

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?

@denisdefreyne
Copy link
Member

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
Copy link
Contributor Author

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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@denisdefreyne
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add alias_method :compiled?, :compiled for extra sweetness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like sweetness.

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

# @return [Boolean]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@denisdefreyne
Copy link
Member

👍 otherwise—looks awesome!

@gjtorikian
Copy link
Contributor Author

Fair enough—it's your project! 🎆

@denisdefreyne
Copy link
Member

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

@denisdefreyne denisdefreyne changed the title [WIP] Implementation of postprocessing Implemeting postprocessing Nov 29, 2015
@denisdefreyne denisdefreyne added this to the 4.1.0 milestone Nov 29, 2015
@denisdefreyne denisdefreyne changed the title Implemeting postprocessing Implement postprocessing Nov 29, 2015
denisdefreyne added a commit that referenced this pull request Nov 29, 2015
@denisdefreyne denisdefreyne merged commit bebc31c into master Nov 29, 2015
@denisdefreyne denisdefreyne deleted the postprocess-life branch November 29, 2015 21:28
@denisdefreyne
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants