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

Add support for resolving relative paths in separate files #316

Merged
merged 7 commits into from
Mar 27, 2019

Conversation

liamrharwood
Copy link
Contributor

This PR is to support resolving relative paths located in a separate file for cases such as HubSpot custom modules.

  • It adds a peek() method to CallStack to allow looking at the top of the include/extend stack in order to determine what file a tag is located in.
  • It adds importResourcePath to the local context of each macro in ImportTag so that the macro will know which file it originated from.

Let me know if additional information is needed.

@liamrharwood liamrharwood requested a review from mattcoley March 25, 2019 21:02
Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

One small nit

@@ -95,6 +95,7 @@ public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) {
interpreter.render(node);
} else {
JinjavaInterpreter child = new JinjavaInterpreter(interpreter);
child.getContext().put("importResourcePath", templateFile);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since jinjava was directly ported from python we generally use snakecase for context variables.

@liamrharwood liamrharwood changed the title Add peek() method to CallStack and add file path to context in ImportTag Add support for resolving relative paths in separate files Mar 26, 2019
@liamrharwood
Copy link
Contributor Author

liamrharwood commented Mar 26, 2019

  • This now uses snake case for import_resource_path
  • I added the import_resource_path to context in FromTag as well
  • I changed ImportTag to use a child interpreter for the import case instead of just import...as. This is necessary so that the macros imported without as can still use import_resource_path to know what file they came from.
  • I updated some tests that were failing. It seemed just like a discrepancy in error messaging, so I think it should be okay.

@codecov-io
Copy link

codecov-io commented Mar 26, 2019

Codecov Report

Merging #316 into master will increase coverage by 0.22%.
The diff coverage is 86.2%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #316      +/-   ##
============================================
+ Coverage     71.25%   71.48%   +0.22%     
- Complexity     1561     1570       +9     
============================================
  Files           239      239              
  Lines          4913     4934      +21     
  Branches        790      793       +3     
============================================
+ Hits           3501     3527      +26     
+ Misses         1129     1124       -5     
  Partials        283      283
Impacted Files Coverage Δ Complexity Δ
...in/java/com/hubspot/jinjava/lib/tag/ImportTag.java 88.09% <100%> (+15.12%) 8 <0> (+1) ⬆️
.../hubspot/jinjava/interpret/JinjavaInterpreter.java 78.97% <100%> (+0.24%) 53 <0> (ø) ⬇️
...n/java/com/hubspot/jinjava/lib/tag/IncludeTag.java 62.5% <100%> (+3.4%) 4 <0> (ø) ⬇️
...in/java/com/hubspot/jinjava/interpret/Context.java 79.77% <100%> (+0.22%) 83 <1> (+2) ⬆️
...java/com/hubspot/jinjava/lib/fn/MacroFunction.java 100% <100%> (ø) 12 <3> (+3) ⬆️
...main/java/com/hubspot/jinjava/lib/tag/FromTag.java 88.88% <100%> (+0.25%) 9 <0> (ø) ⬇️
.../java/com/hubspot/jinjava/interpret/CallStack.java 87.09% <20%> (-12.91%) 14 <1> (+1)
...m/hubspot/jinjava/interpret/TagCycleException.java 70% <0%> (+10%) 6% <0%> (+1%) ⬆️
...pot/jinjava/interpret/ImportTagCycleException.java 100% <0%> (+100%) 1% <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 6f27899...afed74b. Read the comment docs.

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

The functionality of this looks good to me. Can we have tests to ensure this behavior though so someone does not break this in the future?

@liamrharwood
Copy link
Contributor Author

Alright, I changed this around a bit to better support all types of includes, imports, and extends.

  • I added a currentPathStack to Context which keeps track of the current file no matter what kind of tag is used.
  • I still pass the import_resource_path into imported macros but now I push it onto the currentPathStack when evaluating the macro. I do this without checking for cycles in order to allow for macros referencing other macros in the same file.
  • I added a test to ensure that import_resource_path is included in the local context scope for macros.
  • I push the extends path from the top of the extendsPathStack onto the currentPathStack during the processExtendParents step of JinjavaInterpreter::render.

Copy link
Collaborator

@mattcoley mattcoley left a comment

Choose a reason for hiding this comment

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

LGTM

@liamrharwood liamrharwood merged commit 2dc589f into master Mar 27, 2019
@liamrharwood liamrharwood deleted the call-stack-peek branch March 27, 2019 20:18
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.

4 participants