edgedb-js icon indicating copy to clipboard operation
edgedb-js copied to clipboard

ISO 8601 -> `cal::local_date` casting

Open jackfischer opened this issue 1 year ago • 8 comments

ISO 8601 strings are very common. Currently, in order to use them with EdgeDB cal::local_date you have to parse the string and reconstruct the cal::local_date. It would be helpful to just cast strings to cal::local_date, similar to the existing behavior for std::uuid.

jackfischer avatar Jun 05 '23 21:06 jackfischer

Why not use to_local_date()?

elprans avatar Jun 05 '23 21:06 elprans

Ah I see. I don't fully understand the philosophy of when conversions are functions vs casts (maybe the potential variation in formats is the decision maker here) though I think the uuid behavior is lowest friction.

jackfischer avatar Jun 06 '23 02:06 jackfischer

Could you bring up a few examples of the strings that aren't working?

edgedb> select <cal::local_date>'2023-05-07';
{<cal::local_date>'2023-05-07'}
edgedb> select <cal::local_date>'20230507';
{<cal::local_date>'2023-05-07'}

The basic YYYY-MM-DD format works (with and without the dashes). Are you trying to use a string that also has a time component? An example would really help clarify what problem you're running into.

vpetrovykh avatar Jun 06 '23 18:06 vpetrovykh

Ah, casts do exist? This problem was occurring in QB.

e.params({example: e.cal.local_date}, $=>...).run(client, {example: "2020-01-01"});

Type 'string' is not assignable to type 'LocalDate'.ts(2322)

jackfischer avatar Jun 10 '23 00:06 jackfischer

In case you just have a string already and you don't want to parse it in the client code the pattern is something like this in EdgeQL:

select Foo filter .date = <cal::local_date><str>$example

Notice the two chained casts here. The one adjacent to the parameter $example is mandatory and is not actually a cast as much as it is a type declaration for the query parameter. This declaration is pretty strict and it basically tells EdgeDB what are you sending over the wire and what it should expect to decode. The outer cast is simply a cast , which will work fine as long as it's legal to cast between the parameter type and whatever new type you specify there.

So you would want to do something along the lines of:

e.params({example: e.str}, params =>
  ... 
  e.cast(e.cal.local_date, params.example) 
  ...
).run(client, {example: "2020-01-01"});

You should use edgedb.LocalDate to instantiate a value for an e.cal.local_date parameter if the raw data you have is already a bunch of integers that are used to compose the date, so that you don't have to parse anything on the client.

We don't have a convenience function to convert a string to an edgedb.LocalDate value in the client code, though.

vpetrovykh avatar Jun 13 '23 17:06 vpetrovykh

Many thanks for all the information. What is the difference between the pattern above and the e.uuid behavior where the cast appears to happen automatically? Is the convenience function you mentioned actually something that would happen silently when a string is passed in as an e.uuid param and is the difference maker?

jackfischer avatar Jun 13 '23 19:06 jackfischer

Hm, could be an oversight. I think all types that support string casts should have a JS constructor function taking a string. But this really an edgedb-js issue now, so moving this thread there.

elprans avatar Jun 13 '23 19:06 elprans

What is the difference between the pattern above and the e.uuid behavior where the cast appears to happen automatically?

I believe this is a case where, under the hood, uuid is represented by string and since we don't do any kind of branding, the type checker has to allow it. I believe the other "string"-typed primitives (that are represented at runtime as strings) like int64 and decimal, for instance, also have this behavior. I'll do a bit more digging here to verify to ensure we have alignment and consistency here.

I think all types that support string casts should have a JS constructor function taking a string.

We do not currently automatically cast strings to the various date types in our codec, but I agree that this would be an ergonomic improvement in general since JSON doesn't have a native date type. I'm not sure I think that we should always support strings in every type that has a cast since that might be surprising in some cases, like passing a commit hash where you typed an input as a number, but the commit hash is a numeric string so it just so happens to type-check due to casting.

I'm going to add this to my backlog of friction points, but in the meantime, I still think that being very explicit here is actually better even if it's a bit more verbose. Mutating data in the database feels like it deserves to be more strict and the boundary that receives these date strings is probably a better place to transform strings into their correct encodable type.

scotttrinh avatar Jun 20 '23 02:06 scotttrinh