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

Update expression resolver to return null instead of blank string. #296

Merged
merged 3 commits into from
Mar 6, 2019

Conversation

mattcoley
Copy link
Collaborator

It doesn't make sense to return an empty string from the expression resolver if the expression cannot be evaluated due to an exception. Some built-in tags will treat "" differently from null. For example for will treat "" as [""] whereas it should be treated as [].

@mattcoley
Copy link
Collaborator Author

I could not find a reason why we return empty string here. We used to return false and then that was updated to empty string. The tags that use the expression resolver handle the null value better than empty string.

@mattcoley mattcoley requested a review from boulter March 5, 2019 21:19
Copy link
Contributor

@jboulter jboulter left a comment

Choose a reason for hiding this comment

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

When was it updated? The only the reason I can think of is that if you're echoing the result back out, we'd rather return an empty string to the page rather than "null".

@codecov-io
Copy link

Codecov Report

Merging #296 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #296      +/-   ##
============================================
+ Coverage     71.96%   71.98%   +0.02%     
- Complexity     1525     1526       +1     
============================================
  Files           237      237              
  Lines          4769     4769              
  Branches        757      757              
============================================
+ Hits           3432     3433       +1     
  Misses         1071     1071              
+ Partials        266      265       -1
Impacted Files Coverage Δ Complexity Δ
...ava/com/hubspot/jinjava/el/ExpressionResolver.java 81.96% <100%> (ø) 16 <0> (ø) ⬇️
.../main/java/com/hubspot/jinjava/lib/tag/ForTag.java 87.3% <0%> (+1.58%) 20% <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 a807379...ebcc216. Read the comment docs.

@mattcoley
Copy link
Collaborator Author

Here: 1d694cd. The expression resolver is only used in tags: do, for, if, macro, etc. and in the ExpressionNode https:/HubSpot/jinjava/blob/master/src/main/java/com/hubspot/jinjava/tree/ExpressionNode.java#L42. The expression node handles the null value fine by defaulting to empty string.

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.

3 participants