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

Allow accessing :pre snapshot inside layout #548

Merged
merged 2 commits into from May 4, 2015

Conversation

Projects
None yet
2 participants
@ddfreyne
Member

ddfreyne commented May 1, 2015

Second part of the fix for #537.

This PR has two commits:

  1. The fix itself, which consists of recording the :pre snapshot in the snapshots instance variable (which is filled in only with data from explicit snapshots), and let #compiled_content use the snapshots variable to determine whether the :pre snapshot is usable.
  2. Some cleanup (e.g. renaming snapshot to snapshot_name to avoid confusion).

The code related to the fix isn’t as nice as it could be:

  • Ideally, I’d redo the snapshots instance variable to be a hash instead of an array (not sure why it is an array in the first place) but that breaks backwards compatibility in the API. :(
  • The snapshots instance variable is a bit weird anyway. It is used to check whether #compiled_content is trying to access a snapshot that exists, but I believe @content can be used for that purpose.
  • I would also love snapshots to be a first-class concept. Nanoc::Snapshot would be great—instead of snapshot[0], we’d have snapshot.name, and instead of snapshot[1], we’d have snapshot.final?.
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 1, 2015

Member

@jethrogb This is the fix you’re looking for, I believe.

Member

ddfreyne commented May 1, 2015

@jethrogb This is the fix you’re looking for, I believe.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 3, 2015

Member

CC @nanoc/contributors — would be great to have a review, so this can end up in the upcoming release!

Member

ddfreyne commented May 3, 2015

CC @nanoc/contributors — would be great to have a review, so this can end up in the upcoming release!

@jethrogb

This comment has been minimized.

Show comment
Hide comment
@jethrogb

jethrogb May 4, 2015

Thanks for this PR. Works perfectly for me (I tested against 3.7.5 applying both #538 and this)

jethrogb commented May 4, 2015

Thanks for this PR. Works perfectly for me (I tested against 3.7.5 applying both #538 and this)

@ddfreyne ddfreyne removed the to review label May 4, 2015

ddfreyne added a commit that referenced this pull request May 4, 2015

Merge pull request #548 from nanoc/pre-snapshot-in-layout
Allow accessing :pre snapshot inside layout

@ddfreyne ddfreyne merged commit 887ace7 into release-3.7.x May 4, 2015

2 checks passed

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

@ddfreyne ddfreyne deleted the pre-snapshot-in-layout branch May 4, 2015

@ddfreyne ddfreyne modified the milestone: 3.7.6 May 4, 2015

@jethrogb

This comment has been minimized.

Show comment
Hide comment
@jethrogb

jethrogb May 4, 2015

The snapshots instance variable is a bit weird anyway. It is used to check whether #compiled_content is trying to access a snapshot that exists, but I believe @content can be used for that purpose.

Quite weird, I feel like there's something missing. I grepped through the code, and it looks like the only uses of this variable are https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/result_data/item_rep.rb#L242 and
https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/result_data/item_rep.rb#L435 and
https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/compilation/compiler.rb#L330 . In particular, manually calling ItemRep.snapshot does not change this variable. Is this wanted behavior?

jethrogb commented May 4, 2015

The snapshots instance variable is a bit weird anyway. It is used to check whether #compiled_content is trying to access a snapshot that exists, but I believe @content can be used for that purpose.

Quite weird, I feel like there's something missing. I grepped through the code, and it looks like the only uses of this variable are https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/result_data/item_rep.rb#L242 and
https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/result_data/item_rep.rb#L435 and
https://github.com/nanoc/nanoc/blob/release-3.7.x/lib/nanoc/base/compilation/compiler.rb#L330 . In particular, manually calling ItemRep.snapshot does not change this variable. Is this wanted behavior?

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 4, 2015

Member

@jethrogb Yeah. It is confusing, I have to admit.

The @snapshots instance var is filled in by Nanoc::Compiler#compile_reps. It runs through the Rules file, detects all snapshots and stores them inside Nanoc::ItemRep instances. The names of the snapshots are used solely to detect whether the snapshot given to #compiled_content is one that exists.

This is definitely a candidate for refactoring!

Member

ddfreyne commented May 4, 2015

@jethrogb Yeah. It is confusing, I have to admit.

The @snapshots instance var is filled in by Nanoc::Compiler#compile_reps. It runs through the Rules file, detects all snapshots and stores them inside Nanoc::ItemRep instances. The names of the snapshots are used solely to detect whether the snapshot given to #compiled_content is one that exists.

This is definitely a candidate for refactoring!

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