google / jsonnet

Jsonnet - The data templating language
http://jsonnet.org
Apache License 2.0
7.02k stars 441 forks source link

Extending Strings with Dictionaries doesn't raise an error? #754

Open lihaoyi opened 4 years ago

lihaoyi commented 4 years ago

I just noticed that this works on the online jsonnet.org scratchpad:

input.jsonnet

"hello world" {"key": "value"}

output.json

"hello world{\"key\": \"value\"}"

In databricks/sjsonnet, this instead throws the following exception:

sjsonnet.Error: Expected object, found string
    at .(foo.jsonnet:1:15)

Presumably we should make google/jsonnet throw an exception as well? I don't really see any way this usage pattern is useful, so if it appears in code it seems very likely to be a programmer error

sbarzowski commented 4 years ago

The spec says: "String concatenation will implicitly convert one of the values to a string if necessary." I agree that it is not ideal. I considered changing it, but we didn't want to break the existing users.

BTW It can be useful for example for error messages and such:

error "expected x to be a string, but got " + x

Of course you could just explicitly call std.toString(x) which is only slightly more verbose.

lihaoyi commented 4 years ago

Oh I see what’s happening, you guys are desugaring a{...} to a + {...}, whereas I’m treaming a{...} separately as a special form.

On Thu, 2 Jan 2020 at 6:11 PM, Stanisław Barzowski notifications@github.com wrote:

The spec says: "String concatenation will implicitly convert one of the values to a string if necessary." I agree that it is not ideal. I considered changing it, but we didn't want to break the existing users.

BTW It can be useful for example for error messages and such:

error "expected x to be a string, but got " + x

Of course you could just explicitly call std.toString(x) which is only slightly more verbose.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/google/jsonnet/issues/754?email_source=notifications&email_token=AAHEB7C4CXCU2ETETNIJ5S3Q3W4TZA5CNFSM4KB554J2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH6ANNA#issuecomment-570164916, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHEB7FKJHRJGDKPQMEVQKLQ3W4TZANCNFSM4KB554JQ .

sbarzowski commented 4 years ago

Hmmm... I was considering separating the two for another reason (https://github.com/google/go-jsonnet/issues/233). And I think it's very reasonable to disable implicit string conversion when using the syntax designed for inheritance.

I'm in favor of doing it your way in google/jsonnet. Technically it breaks the compatibility, but I don't think many people would be affected.

lihaoyi commented 4 years ago

Yeah I'm fine with anything. It's not a huge deal, just a weird behavior I noticed, not actually causing us any issues either

sparkprime commented 4 years ago

Implicit string concatenation is really useful when building error messages and such. I think "Object: " + { } should therefore be allowed. On the other hand "Object: " { } is pretty weird. I'd be ok with disallowing that... However we might just go the full hog and try to move away from the implicit + in all cases since it causes people grief when they're trying to make a list of objects and forget the ,.

lihaoyi-databricks commented 4 years ago

However we might just go the full hog and try to move away from the implicit + in all cases since it causes people grief when they're trying to make a list of objects and forget the ,.

That would be wonderful. Forgetting , in a list of objects causes people on our team grief on a regular basis.

This is especially common because trailing commas are optional, so it's easy to add a new object on a new line in a big list, not realize the previous last-line didn't have a trailing comma. Since objects in lists tend to have the same set of keys, the newly object acts as an extension that nicely clobbers all of the previous object's keys. This gives exactly zero indication that something is wrong except for the now second-to-last object disappearing from the JSON output!

If someone actually wants to extend an object, adding a + is straightforward and no great hardship