-
Notifications
You must be signed in to change notification settings - Fork 170
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
Fixes #159: Adds dict support for DefaultFilter #386
Conversation
79d1362
to
5c495af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me with a few minor changes.
String... args) { | ||
Object... args) { | ||
|
||
List<String> stringArgs = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do this all in one loop. In fact, you don't need stringArgs
at all. I know this was just moved from another file, but might as well clean it up while we're in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
if (arg.length < 1) { | ||
throw new TemplateSyntaxException(interpreter, getName(), "requires 1 number (divisor) argument"); | ||
} | ||
String toMul = arg[0]; | ||
String toMul = arg[0] == null ? null : Objects.toString(arg[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects.toString
takes care of nulls for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs state that it returns "null"
and not null
.
Also if I use Objects.toString(arg[0])
then the following test breaks. Therefore had to ensure that arg[0] == null
check was made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know.
@@ -61,7 +64,7 @@ public Object filter(Object object, JinjavaInterpreter interpreter, String... ar | |||
return object; | |||
} | |||
|
|||
return args[0]; | |||
return (args[0] instanceof PyWrapper) ? args[0] : Objects.toString(args[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the only real significant change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. basically checking if the argument is an instance of PyWrapper then we do not need to convert it into String.
7fd6830
to
ea62429
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
adds a fix for #159
prev discussion : #344