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

Wrap items in for loop with "PyIsh" equivalents #202

Merged
merged 6 commits into from
Jun 13, 2018
Merged

Conversation

jessbrandi
Copy link
Contributor

@jessbrandi jessbrandi commented Jun 8, 2018

The wrap code in JinjavaInterpreterResolver, wraps objects in their Pyish* counterparts. However, if the value being wrapped was a list, the children of that value were not wrapped. This resulted in Date values being treated differently if they appeared as a single value or in a list.

My initial attempt was modify the list to wrap the children in the wrap method, but that resulted in issues modifying the list later on using set and append functions.

@jessbrandi jessbrandi changed the title Recursively wrap objects for lists and maps Wrap items in for loop with "PyIsh" equivalents Jun 8, 2018
@codecov-io
Copy link

codecov-io commented Jun 8, 2018

Codecov Report

Merging #202 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #202      +/-   ##
===========================================
+ Coverage     71.24%   71.3%   +0.06%     
- Complexity     1348    1351       +3     
===========================================
  Files           215     215              
  Lines          4278    4280       +2     
  Branches        682     682              
===========================================
+ Hits           3048    3052       +4     
+ Misses          988     986       -2     
  Partials        242     242
Impacted Files Coverage Δ Complexity Δ
.../main/java/com/hubspot/jinjava/lib/tag/ForTag.java 77.77% <100%> (ø) 14 <0> (ø) ⬇️
.../hubspot/jinjava/interpret/JinjavaInterpreter.java 85.51% <100%> (+0.1%) 47 <1> (+1) ⬆️
...ava/com/hubspot/jinjava/el/ExpressionResolver.java 89.36% <100%> (+0.23%) 9 <1> (+1) ⬆️
...va/com/hubspot/jinjava/objects/date/PyishDate.java 51.61% <0%> (+6.45%) 10% <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 cc8c477...3a4e721. Read the comment docs.

Copy link
Contributor

@nbelisle11 nbelisle11 left a comment

Choose a reason for hiding this comment

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

Not terribly familiar with all the implications here but seems safe enough to me

public void testForLoopWithDates() {
Map<String, Object> context = Maps.newHashMap();
Date testDate = new Date();
context.put("the_list", Lists.newArrayList(new Date()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to use testDate when initializing the_list right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good catch. I guess this test worked anyway, because PyishDates aren't that precise? (toString only prints out to seconds, not millis).

@@ -129,6 +130,7 @@ public String interpret(TagNode tagNode, JinjavaInterpreter interpreter) {
LengthLimitingStringBuilder buff = new LengthLimitingStringBuilder(interpreter.getConfig().getMaxOutputSize());
while (loop.hasNext()) {
Object val = loop.next();
val = interpreter.resolveProperty(val, Collections.emptyList());
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you're trying to do here, but it's weird to call resolveProperty with an empty list of properties to resolve. Perhaps it would make more sense just to add a delegate method in JinjavaInterpreter for wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 updated

@jessbrandi jessbrandi merged commit 90d1aa1 into master Jun 13, 2018
@jessbrandi jessbrandi deleted the wrap-child-dates branch June 13, 2018 17:57
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