amazon-ion / ion-element-kotlin

IonElement is an immutable in-memory representation of the Ion data format. IonElement's API is idiomatic to Kotlin.
Apache License 2.0
8 stars 8 forks source link

Add Primitive Operators #80

Closed tjcsrc16 closed 1 year ago

tjcsrc16 commented 1 year ago

Issue #, if available: N/A

Description of changes:

This adds operator overloads as extension functions between various IonElement subtypes and primitive types. The purpose is to improve the usability of working directly with IonElement values rather than serializing to/from data classes. Specifically, the following operators are supported:

For mixed-type operators, the primitive value can be on the left or right and the result will always be an IonElement value. For example, ionInt(1) + 2 == ionInt(3) == 2 + ionInt(1).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tjcsrc16 commented 1 year ago

It looks like the compliance test failure happens in a fresh clone when pulling the latest commit of ion-tests.

popematt commented 1 year ago

This looks like a potentially useful idea, but there are a few issues to sort out first.

Annotations: Ion values have annotations, and there isn't really a good way to do anything with them that will not be surprising to someone. This is especially complicated by the fact that annotations are ordered and can be repeated. For example:

A possible solution would be to only define the math operators such that a <ion> + <primitive> = <ion> and/or <ion> + <primitive> = <primitive>, but no implementations of <ion> + <ion>. Then we can say that the operator only operates on the value component of the Ion value.

Consistency of compareTo() and equals(): IonElement already has an implementation of equals() that uses Ion data model equivalence. If we implement compareTo() as provided, we will end up with logical inconsistencies, like this:

A possible solution would be to create separate infix functions such as eq, ne, gt, gte, etc. Those functions wouldn't have to worry about whether they follow Ion equivalence, and we ensure that we don't have inconsistent equals() and compareTo().

Given the potential for surprising behavior, I think this should probably be a "requires opt-in" API, at least for the time being.

tjcsrc16 commented 1 year ago

A possible solution would be to only define the math operators such that a <ion> + <primitive> = <ion> and/or <ion> + <primitive> = <primitive>, but no implementations of <ion> + <ion>

I think that is a good solution to unblock adding the feature. I would prefer to always return an <ion> value and not make it dependent on the ordering of the types, since the intention is to work with <ion> values. Also, it will be awkward for non-commutative operators, e.g., if you wanted to subtract an <ion> from a <primitive> and get an <ion> you would have to add the negative of the <ion> or promote the result manually.

Consistency of compareTo() and equals()

I will remove the compareTo and implement the infix operators mentioned.

popematt commented 1 year ago

I would prefer to always return an value and not make it dependent on the ordering of the types, since the intention is to work with values.

Sorry I wasn't very clear—I had an unstated assumption that the ordering of the Ion and primitive values would not matter.

I do agree with you. The binary (two-arg) operators could be defined for cases where there is one Ion value and one primitive, and it could return an Ion value or a primitive value.