Skip to content

defer import tag & improved reconstructImage#333

Merged
jkollmann merged 9 commits into
masterfrom
defer-import-tag-and-reconstruct-nested
May 31, 2019
Merged

defer import tag & improved reconstructImage#333
jkollmann merged 9 commits into
masterfrom
defer-import-tag-and-reconstruct-nested

Conversation

@jkollmann

Copy link
Copy Markdown
Contributor

Defer import tag

This change handles the case where an import tag depends on deferred values. In this case the nodes inside of the tags will be added to the deferredNodes list and all created values inside of the SetTag will be deferred.

Assuming the file my_properties.html depends on a deferred value.

Example 1: {% import 'custom/page/web_page_basic/my_properties.html' as header_footer %}
header_footer will be deferred

Example 2: {% import 'custom/page/web_page_basic/my_properties.html' %}
All variables defined (or deferred) inside of the file will be deferred.

After that a DeferredValueException is thrown from inside of the ImportTag in order to avoid rendering it and to reconstruct it properly.

Improved reconstructImage

This change adds reconstructImage as a public function on the Node base class. This allows the child classes of Node to implement this function, however it will default to master.getImage() so that users are not forced to implement it when they extend Node. This way reconstructImage can be improved inside of TagNode to reconstruct recursively. Before it would only reconstruct at the depth of 1.

Example (from a test added in DeferredTest):

{% if deferred %}{% if resolved %}{{resolved}}{% endif %}{% else %}b{% endif %}

Before the change this would have been reconstructed to

{% if deferred %}{% if resolved %}{% else %}b{% endif %}

After the change it reconstructs correctly to the original template.

Considerations

At the moment, macros that are called with/depend on deferred values are not preserved correctly. At the time of rendering the macro definition, it's not possible right now to know if the value it depends on will be defined when the macro is actually called. This is not trivial and I'll need to think more about how to make it work well, but in order to move forward and make progress we've decided to not support this for the time being.

Also I'm aware of the FromTag, which is very similar to the ImportTag. I'll add DeferredValue support for it later.

@jkollmann jkollmann requested review from boulter and pfarrel May 28, 2019 10:59

@pfarrel pfarrel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.
It might be nice to add a comment to the deferred login in ImportTag to explain a bit what's going on. It makes sense to me now, but I'm sure it could be surprising to someone who wasn't familiar with those features.

@codecov-io

codecov-io commented May 30, 2019

Copy link
Copy Markdown

Codecov Report

Merging #333 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #333      +/-   ##
============================================
+ Coverage     71.54%   71.63%   +0.09%     
- Complexity     1580     1588       +8     
============================================
  Files           239      239              
  Lines          4965     4974       +9     
  Branches        805      807       +2     
============================================
+ Hits           3552     3563      +11     
+ Misses         1126     1125       -1     
+ Partials        287      286       -1
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/com/hubspot/jinjava/tree/TagNode.java 76.31% <100%> (+2.63%) 11 <1> (+2) ⬆️
...in/java/com/hubspot/jinjava/lib/tag/ImportTag.java 84% <100%> (+3.04%) 11 <2> (+4) ⬆️
src/main/java/com/hubspot/jinjava/tree/Node.java 53.57% <100%> (+1.71%) 8 <1> (+1) ⬆️
...in/java/com/hubspot/jinjava/interpret/Context.java 81.05% <0%> (+0.52%) 88% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 693c157...2df59e0. Read the comment docs.

@boulter boulter requested a review from mattcoley May 30, 2019 17:12

@mattcoley mattcoley left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@jkollmann jkollmann merged commit 7eb414a into master May 31, 2019
@jkollmann jkollmann deleted the defer-import-tag-and-reconstruct-nested branch May 31, 2019 08:49
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.

5 participants