search

bkiers / Liqp

An ANTLR based 'Liquid Template' parser and rendering engine.
MIT License
165 stars 94 forks source link

Move Date filter closer to parity with Liquid implementation #208

Closed aschott-looker closed 3 years ago

aschott-looker commented 3 years ago
aschott-looker commented 3 years ago

cc @bkiers

bkiers commented 3 years ago

Hi @aschott-looker, thanks for the PR! Note that @msangel is currently working on date formatting here https://github.com/bkiers/Liqp/pull/207, so I don't think this can be merged before his work is done (and this branch is also failing on Travis CI...)

msangel commented 3 years ago

In my branch the number of supported formats are bigger than before. Also it follows the same as ruby semantic - if some time part is missing, the current time is assumed. I will review this commit and will take an important parts from this to that branch. Thanks! Work is still in progress(mostly on weekends).

aschott-looker commented 3 years ago

@bkiers @msangel -- Sounds reasonable to me.

Since you're working on this part of the code anyway, another thing I would point out is that SimpleDateFormat is not thread-safe. Its usage in liqp.filters.Date could be problematic if used in a multi-threaded environment.

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/time/FastDateFormat.html is a nice drop in replacement that is at parity with SimpleDateFormat but is actually thread-safe.

Another option would be to use DateTimeFormatter but that'd only be supported for Java 8+ and would require a lot more changes than FastDateFormat.

EDIT: Looks like you're already doing that @msangel. Awesome!