Support more date formats in the date filter

[johannhof]

In https://github.com/cobalt-org/liquid-rust/blob/master/src/filters.rs#L308 we use a custom date format to parse dates for the date filter if it's a string.

The original liquid has a to_date function that can take different types of input and parse it. We might even parse different date formats that way https://github.com/Shopify/liquid/blob/master/lib/liquid/utils.rb#L63

[johannhof]

That would also be a good opportunity to add support for the magic "today" and "now" strings, see https://shopify.github.io/liquid/filters/date/

[epage]

If the liquid syntax allows this, I think it'd be nice to read now and today from the Context and fallback to using the current time if they aren't there or aren't compatible.

This would be a bit non-conformant but would be a great for testing.

[djwf]

That would be interesting, but I'm not sure how to make that work within the filter. Conceptually, we'd need to have "today" and "now" be special terms that would be rendered external to the filter if they exist within the Context, and only be handled by the filter if they are not defined within the Context. Is that what you were thinking?

[epage]

For cobalt-org/cobalt.rs#349, I looked up how shopify's date does its parsing:

date calls Utils.to_date (referenced above by johannhof)

• Utils.to_date
  • If already a date time, no need to parse
  • If looks like an integer, parse as timestamp
  • If looks like a string, downcase and use Time.parse which is implemented by date__parse (upstream without syntax highlighting).
    • This supports a lot of different formats, subsets, etc.

The remaining question is what should serde support do. Do we support a single format or accept any format?

First, this assumes we add a DateTime variant to Value. Second, I'm assuming that this is an implementation detail that allows for optimizations (avoid reparsing), so the answer doesn't matter too much. I'd lean to us only serializing to a single format but allow coercing

My thought is that if we add a DateTime to the Value enum, then we serialize/deserialize with a specific format but when we coerce a string to a DateTime, we support everything Liquid does.

[ticky]

Would a PR adding at least a few more ISO8601/RFC3339 date formats to the formats Liquid will try be accepted?

I realise the ultimate goal is to accept anything Ruby’s Time.parse does, but I suspect a suite of common, standard date formats would get most of the way there, without anything other than Chrono’s own parser.

[epage]

As long as they are formats supported by the Ruby implementation, go for it. We don't have to tackle this all at once.

[djwf]

I'm working with this now, and wanted some feedback on the formats I'm adding. The following formats are going into the parse_date_time function.

YYYY-MM-DD HH:MM:SS
DD Month YYYY HH:MM:SS
DD Mon YYYY HH:MM:SS
MM/DD/YYYY HH:MM:SS
Dow Mon DD HH:MM:SS YYYY

Those will be supported both with and without offsets. For those without offsets, the timezone will be set to the local offset (of the host on which the program is running). For those with offsets, only the forms +HHMM and -HHMM will be accepted. In the future, the forms +HH:MM and -HH:MM may be accepted, as well as offset seconds.

Also, I'm adding in "today" and "Today" as synonyms for "now".

To support local offsets, the feature "local-offset" must be enabled for the time dependency in the Cargo.toml to allow non-offset versions to be parsed (and local offsets to be calculated).

Any questions, comments, critiques are welcome. Hopefully I'll have something to throw into a PR within the next few days.

[epage]

Let's start with defaulting to +0000 instead of local time. Maybe I'm confusing this with cobalt but I think we already default to +0000 in other parts. There are also a lot of problems surrounding local offsets that they will be nearly impossible to use in a library (which is why its behind a feature flag).

[djwf]

I don't think you're confusing it with cobalt - we do default to UTC everywhere for now.