Fixes #159 : Adds dict support for DefaultFilter by gabru-md · Pull Request #344 · HubSpot/jinjava

gabru-md · 2019-06-25T06:17:43Z

Fixes #159

Adds a method for processing the dictionary type in DefaultFilter.

Problem : Arguments were being converted to String [] before being processed by the DefaultFilter

Solution : Created a new method for processing the Object [], incase any arg is of type PyWrapper. Implemented the function and created tests for the same.

😄

gabru-md · 2019-06-25T06:25:53Z

boulter

Thanks @gabru-md!

I have a bunch of style comments and some implementation related comments. Let me know if you have any questions. Thanks again!

boulter · 2019-06-25T21:59:34Z

 **********************************************************************/
 package com.hubspot.jinjava.lib.filter;

-import java.math.BigDecimal;


could you update your import order settings? java imports first is our preference

boulter · 2019-06-25T22:00:18Z

 import com.hubspot.jinjava.interpret.InvalidReason;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.interpret.TemplateSyntaxException;
+import com.hubspot.jinjava.interpret.InvalidInputException;


this was moved out of alphabetical order

boulter · 2019-06-25T22:02:05Z

+ * Created by manishdevgan on 25/06/19.
+ */
+public class DefaultFilterTest {
+    Jinjava jinjava;


our indentation standard is 2 spaces. Could you adjust?

boulter · 2019-06-25T22:02:56Z

+
+    @Test
+    public void itSetsDefaultStringValues() {
+        assertThat(jinjava.render("{% set d=d | default(\"some random value\") %}{{ d }}", new HashMap<>())).isEqualTo("some random value");


Could you either leave these as long lines or hard-wrap them on a logical boundary, like , (preferred)?

boulter · 2019-06-25T22:10:10Z

    List<String> stringArgs = new ArrayList<>();

    for (Object arg : allArgs) {
+      if(arg instanceof PyWrapper) {


space after if please

gabru-md · 2019-06-26T05:51:41Z

@boulter I've made changes to the files.
please review again 😄

gabru-md · 2019-06-28T12:26:01Z

cc : @boulter

boulter

Looks like some of the issue have been addressed, but there's still a lot of import ordering changes and diffs that are unnecessary. Could you address those? That should dramatically decrease the number of changes here and it make it easier to review.

boulter · 2019-06-28T14:13:12Z

 import com.hubspot.jinjava.interpret.InvalidReason;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;

+


can you remove extra space here?

boulter · 2019-06-28T14:13:44Z

 **********************************************************************/
 package com.hubspot.jinjava.lib.filter;

+import java.util.HashMap;


still some lingering import reordering...

boulter · 2019-06-28T14:14:15Z

    }

-    return interpreter.resolveProperty(var, args[0]);
+    return interpreter.resolveProperty(var, args[0].toString());


Objects.toString is safer

boulter · 2019-06-28T14:14:54Z

  public Object filter(Object var, JinjavaInterpreter interpreter,
-      String... args) {
-
+      Object... args) {


can you join this with the previous line?

boulter · 2019-06-28T14:15:13Z

-
+      Object... args) {
+    List<String> stringArgs = new ArrayList<>();
+    for(Object arg: args) {


space after for and before : please

boulter · 2019-06-28T14:16:12Z

+      Object... args) {
+    List<String> stringArgs = new ArrayList<>();
+    for(Object arg: args) {
+      stringArgs.add(arg == null ? null : Objects.toString(arg));


Objects.toString(arg) will handle nulls for you.

boulter · 2019-06-28T14:17:16Z

-  public String getName() {
-    return "unixtimestamp";
-  }
+    @Override


please restore original indentation

boulter · 2019-06-28T14:17:50Z

  @Override
  public Object filter(Object var, JinjavaInterpreter interpreter,
-      String... args) {
+                       Object... args) {


can join with previous line

gabru-md · 2019-06-30T17:36:21Z

sure! will do the necessary changes

gabru-md · 2019-07-01T10:04:55Z

@boulter I've added changes. please review again 🙏

boulter

I noticed a bunch of small changes, but there are many more instances that I didn't get to. Can you clean these up first and then I'll take a look when the diffs are smaller? Thanks.

boulter · 2019-07-01T13:55:31Z

      throw new TemplateSyntaxException(interpreter, getName(), "requires 1 number (divisor) argument");
    }
-    String toMul = arg[0];
+    String toMul = (arg[0] == null) ? null : Objects.toString(arg[0]);


Objects.toString will take care of nulls for you.

boulter · 2019-07-01T13:55:50Z

 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.interpret.TemplateSyntaxException;

+


remove extra newlines please

boulter · 2019-07-01T13:56:11Z

 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.util.ForLoop;
 import com.hubspot.jinjava.util.ObjectIterator;
+import org.apache.commons.lang3.math.NumberUtils;


fix reordered import

boulter · 2019-07-01T13:56:20Z

 import com.hubspot.jinjava.lib.fn.Functions;
 import com.hubspot.jinjava.objects.date.PyishDate;

+


extra space

boulter · 2019-07-01T13:56:27Z

        return str.equals("1") ? Boolean.TRUE : BooleanUtils.toBoolean(str);
    }

+


extra space

boulter · 2019-07-01T13:56:40Z

 import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
 import com.hubspot.jinjava.interpret.TemplateSyntaxException;
+import org.apache.commons.lang3.StringUtils;


import order

boulter · 2019-07-01T13:57:23Z

 import com.hubspot.jinjava.doc.annotations.JinjavaParam;
 import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;
+import org.apache.commons.lang3.BooleanUtils;


import order

boulter · 2019-07-01T13:57:42Z

 import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
 import com.hubspot.jinjava.interpret.JinjavaInterpreter;

+


extra space

boulter · 2019-07-01T13:58:05Z

 public class DifferenceFilter extends AbstractSetFilter {

-  @Override
+    @Override


indentation is off here.

gabru-md · 2019-07-14T10:22:54Z

@boulter I'll add changes to this PR in the weekend.
I'm a bit busy with some stuff. Writing it down so that this PR doesn't get closed for inactivity.
cheers!

raomuyang · 2019-07-23T05:15:50Z

@boulter I've created a checkstyle configuration to cover some scenarios as much as possible: #353

gabru-md · 2020-01-15T21:10:01Z

opening a different PR for the same.

gabru-md added 2 commits June 25, 2019 11:42

Adds dict support for DefaultFilter

65dfa79

adds docstring

a5f6228

might fix tests

c74e60e

boulter requested changes Jun 25, 2019

View reviewed changes

adds changes

471e869

boulter requested changes Jun 28, 2019

View reviewed changes

fix

8906d2f

boulter requested changes Jul 1, 2019

View reviewed changes

raomuyang mentioned this pull request Jul 22, 2019

Add codeStyleChecker for jinjava #353

Merged

gabru-md closed this Jan 15, 2020

gabru-md mentioned this pull request Jan 15, 2020

Fixes #159: Adds dict support for DefaultFilter #386

Merged

		import com.hubspot.jinjava.interpret.InvalidReason;
		import com.hubspot.jinjava.interpret.JinjavaInterpreter;

		import com.hubspot.jinjava.interpret.JinjavaInterpreter;
		import com.hubspot.jinjava.interpret.TemplateSyntaxException;

		import com.hubspot.jinjava.lib.fn.Functions;
		import com.hubspot.jinjava.objects.date.PyishDate;

		return str.equals("1") ? Boolean.TRUE : BooleanUtils.toBoolean(str);
		}

		import com.hubspot.jinjava.doc.annotations.JinjavaSnippet;
		import com.hubspot.jinjava.interpret.JinjavaInterpreter;

Uh oh!

Conversation

gabru-md commented Jun 25, 2019

Uh oh!

gabru-md commented Jun 25, 2019

Uh oh!

boulter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabru-md commented Jun 26, 2019

Uh oh!

gabru-md commented Jun 28, 2019

Uh oh!

boulter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabru-md commented Jun 30, 2019

Uh oh!

gabru-md commented Jul 1, 2019

Uh oh!

boulter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabru-md commented Jul 14, 2019

Uh oh!

raomuyang commented Jul 23, 2019

Uh oh!

gabru-md commented Jan 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development