Filter upgrades to support kw params#531
Conversation
…. Null parameter value is treated as missing
…to AbstractFilter
join filter: `attr` renamed to `attribute` matching unit test
datetimeformat: fix function jinjavadoc and param defaults
boulter
left a comment
boulter
left a comment
There was a problem hiding this comment.
I like it. It seems to clean up a lot of code.
| String format = (String) parsedArgs.get(FORMAT_PARAM); | ||
| String timezone = (String) parsedArgs.get(TIMEZONE_PARAM); | ||
| String locale = (String) parsedArgs.get(LOCALE_PARAM); | ||
| if (format == null && timezone == null && locale == null) { |
There was a problem hiding this comment.
Must all of these be null?
There was a problem hiding this comment.
I could probably fix up the Functions code, but currently it errors out on zone being null. Let me know if you are happy for me to change it
| ) | ||
| public class DateTimeFormatFilter implements Filter { | ||
| public class DateTimeFormatFilter extends AbstractFilter implements Filter { | ||
| public static final String FORMAT_PARAM = "format"; |
There was a problem hiding this comment.
I really appreciate the static constants!
| try { | ||
| length = Integer.parseInt(Objects.toString(args[0])); | ||
| } catch (Exception e) { | ||
| ENGINE_LOG.warn( |
There was a problem hiding this comment.
wow, this removes a lot of code! Can you ensure that this is still covered in the unit tests?
There was a problem hiding this comment.
Ah I see your point, it ignores parsing errors. Given that we are using defaultValue now did you want to keep parse-and-ignore-error functionality or rely on correct user input/sensible default value?
There was a problem hiding this comment.
I think we should just confirm that it still validates the value but we don't need to log a warning. It's pretty useless.
There was a problem hiding this comment.
Take a look at the last commits, I've added a default on error. Additionally I've fixed up the number parsing as NumberUtils seems to default to 0 if not parsed.
…ead of returning default 0
…ead of returning default 0. Expose defaultValue for parameter name
2 participants
This change includes the following:
@boulter This is following up on the
AbstractFilter#523 , these are draft changes so let me know what you think of these changes and whether you'd prefer split PRs tooJinjavadoc alignment with tests includes:
number=>intchanges where filter actually required an integer - the conversion is done in AbstractFilter