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

Implement nested properties for selectattr and rejectattr filters. #238

Merged
merged 5 commits into from
Oct 8, 2018

Conversation

mattcoley
Copy link
Collaborator

Continuation of #183 to make these filters expressive. Previously if you had a nested object there would be no way to selectattr on the nested property, thereby requiring the need to use a for loop or make another API call. This changes selectattr to use the chain property resolver and modifies rejectattr to share code with selectattr.

@mattcoley mattcoley requested a review from boulter October 8, 2018 18:19
@codecov-io
Copy link

codecov-io commented Oct 8, 2018

Codecov Report

Merging #238 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #238      +/-   ##
===========================================
- Coverage      71.6%   71.6%   -0.01%     
+ Complexity     1378    1375       -3     
===========================================
  Files           218     218              
  Lines          4350    4331      -19     
  Branches        689     683       -6     
===========================================
- Hits           3115    3101      -14     
+ Misses          999     996       -3     
+ Partials        236     234       -2
Impacted Files Coverage Δ Complexity Δ
...m/hubspot/jinjava/lib/filter/RejectAttrFilter.java 100% <100%> (+21.73%) 3 <2> (-4) ⬇️
...m/hubspot/jinjava/lib/filter/SelectAttrFilter.java 70.37% <100%> (+1.13%) 8 <1> (+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 649bda5...bbc12b6. Read the comment docs.

Copy link
Contributor

@boulter boulter left a comment

Choose a reason for hiding this comment

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

Looks OK mod one comment. What is the problem this is solving? Some examples would help. Do we need to update documentation?

if (expTest.evaluate(attrVal, interpreter, expArgs)) {
Object attrVal = new Variable(interpreter, String.format("%s.%s", "placeholder", attr)).resolve(val);
boolean pass = expTest.evaluate(attrVal, interpreter, expArgs);
if ((acceptObjects && pass) || (!acceptObjects && !pass)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could just do acceptObjects == pass

@mattcoley
Copy link
Collaborator Author

Anthony mentioned he would update the public documentation. When using selectattr on a list of objects, you could only apply an expression test to a top level property. If an object is composed of other objects, you would not be able to filter on those objects' properties.

@boulter
Copy link
Contributor

boulter commented Oct 8, 2018

Ok, I get it now. This allows you to filter by option.id instead of just option for example.

@mattcoley
Copy link
Collaborator Author

Yep 👍 On the HubSpot side, it will encourage people to use HubL instead of multiple API calls to do filtering on HubDB rows.

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