Add Mutable View for Struct Elements #84

tjcsrc16 · 2023-10-23T18:22:45Z

Issue #, if available:

Description of changes:

Adds a MutableStructFields interface which represents an iterable sequence of mutable fields that can be used to construct a StructElement.

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

Adds a MutableStructFields interface which represents an iterable sequence of mutable fields that can be used to construct a StructElement.

codecov · 2023-10-23T18:26:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@247c128). Click here to learn what that means.

Additional details and impacted files

@@            Coverage Diff            @@
##             master      #84   +/-   ##
=========================================
  Coverage          ?   90.29%           
  Complexity        ?      491           
=========================================
  Files             ?       31           
  Lines             ?     1051           
  Branches          ?      153           
=========================================
  Hits              ?      949           
  Misses            ?       59           
  Partials          ?       43

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/com/amazon/ionelement/impl/StructElementImpl.kt

popematt

Thank you so much for following up on this. Overall, this is really helpful, and I think this is the right concept. After seeing some of the demo classes, I think the APIs might need to change just a little bit back in the direction of your original suggestion for an update DSL.

I think this PR is also excellent work to inform the design of similar functions for SexpElement and ListElement, which I can put together after we get this one merged.

popematt · 2023-10-23T18:37:45Z

src/com/amazon/ionelement/api/IonElement.kt

+    /** A mutable shallow copy of this struct's fields */
+    public val mutableFields: MutableStructFields


I am hesitant to make this a val given that val implies that it is a property of the class. Maybe fun to____() instead? (I'm not sure what that___ should be; I suppose it depends on what the final name for MutableStructFields ends up being.)

I agree a function makes more sense since the property always returns a new object anyway.

Another question is whether the MutableStructFields object can or should carry the annotations and metas attached to the original struct.

If it's MutableStructFields, then I would say that it does not need the annotations or the metas. If we were going for a MutableStructElement, then we probably should include the annotations and metas.

I'm not opposed to going for a MutableStructElement, but I think it's a much more drastic change since we would need to define how the mutable elements fit into the inheritance hierarchy (does it implement MutableIonElement, and how do IonElement and MutableIonElement relate?) and we would probably end up implementing mutable versions for all of the Ion types.

popematt · 2023-10-23T18:38:30Z

src/com/amazon/ionelement/api/MutableStructFields.kt

+ */
+public interface MutableStructFields : Iterable<StructField> {
+
+    public val fields: MutableMap<String, MutableList<AnyElement>>


Question—I think this looks like an implementation detail. Is it necessary to include it in the interface?

No, that looks like a mistake, it should have been private.

src/com/amazon/ionelement/api/MutableStructFields.kt

popematt · 2023-10-23T18:44:01Z

src/com/amazon/ionelement/api/MutableStructFields.kt

+    public fun add(field: StructField): MutableStructFields
+
+    /** Removes a random occurrence of a field found with the given name or does nothing if no field exists */
+    public fun remove(fieldName: String): MutableStructFields


Question—do you think there is a real use case for removing one field with a given field name? Perhaps it would be more useful if the signature was fun remove(field: StructField) so that the user of the library can remove a specific field rather than an arbitrary one.

Makes sense, I will update this function.

popematt · 2023-10-23T18:50:10Z

src/com/amazon/ionelement/api/MutableStructFields.kt

+     * If one or more fields with the specified name already exists, this replaces the value of the *first* field found.
+     *
+     * Otherwise, a new field with the given name and value is added to the collection


We should probably not make promises that depend on the order of StructFields. Is there any particular reason that you think this should not replace all existing fields with the given field name? (I think it would be a sensible choice, and it is what you have chosen to do for the setAll function.)

Makes sense as the behavior you describe aligns with ion-java.

popematt · 2023-10-24T00:10:10Z

src/com/amazon/ionelement/api/IonElement.kt

+     *
+     * If multiple fields with the same name exist they are all replaced by the new fields.
+     */
+    public fun copy(fields: Iterable<StructField>): StructElement


So, I know I suggested copy in #83, but my thinking has changed over the last few months. I suppose it's philosophical—is it really a copy if you're doing a full replacement of the fields? (Whether someone is fully replacing the fields or not, that's what this function does.) I don't necessarily have a better name right now, and I'm definitely open to suggestions.

After writing this, I feel that copy is a bit anemic. This seems more straight forward.

myStruct.toMutableStructFields() .add(...) .add(...) ... .toStruct()

There just needs to be a way to deal with the annotations and metas you may want to copy over.

Having a plus operator between two MutableStructFields and between StructElement and MutableStructFields also makes copy less useful.

popematt · 2023-10-24T00:20:49Z

src/com/amazon/ionelement/impl/MutableStructFieldsImpl.kt

+        value as AnyElement
+        if (fieldName in fields) {
+            fields[fieldName]?.clear()
+            fields[fieldName]?.add(value)
+        } else {
+            fields[fieldName] = mutableListOf(value)
+        }
+


Two comments about this.

This does not match the function documentation. (It's replacing all fields with that name rather than one field with that name.) However, I believe this implementation is the better choice, and it is the documentation that should change.

You can simplify this to just

Suggested change

value as AnyElement

if (fieldName in fields) {

fields[fieldName]?.clear()

fields[fieldName]?.add(value)

} else {

fields[fieldName] = mutableListOf(value)

}

val values = fields.getOrPut(fieldName, ::mutableListOf)

values.clear()

values.add(value as AnyElement)

popematt · 2023-10-24T00:39:09Z

src/com/amazon/ionelement/impl/MutableStructFieldsImpl.kt

+    }
+}
+
+public fun buildStruct(body: MutableStructFields.() -> Unit): StructElement {


I think it might be worth adding annotations and possibly metas to this function, as in the ionStructOf() functions.

Also, since this function is public, it should be moved into com.amazon.ionelement.api.

popematt · 2023-10-24T00:40:07Z

src/com/amazon/ionelement/impl/MutableStructFieldsImpl.kt

+    override fun toStruct(annotations: List<String>, metas: MetaContainer): StructElement {
+        return ionStructOf(this, annotations, metas)
+    }
+
+    override fun toStruct(): StructElement {
+        return ionStructOf(this)
+    }


Since the ionStructOf() functions exist, I think these might be redundant. (I want to be careful not to add unnecessary things because once it's out there, it's permanent at least until the next major version.)

I added this because the following:

myStruct.toMutableStructFields() .add(...) .add(...) ... .toStruct()

seems cleaner than:

ionStructOf(myStruct.toMutableStructFields() .add(...) .add(...) ...)

or

myStruct.toMutableStructFields() .add(...) .add(...) ... .let {ionStructOf(it)}

This is just a matter of style though, and should style warrant expanding the interface?

I'm not opposed to ever adding them, but for now, there are other ways to build the struct, and I think it is better to start smaller and then add things if it does end up being an issue.

If we have update() or similar methods on StructElement, then in Java, you could write:

myStruct.updateFields(f -> f.add(...) .set(...));

And in Kotlin:

myStruct.update { add(...) set(...) }

What do you think? Do you prefer the lambda expression (Kotlin) and consumer-builder (Java) or the traditional builder syntax?

I think this is also related to the question of whether annotations should be preserved. I think that if we have an "update" method, the name implies that it would preserve annotations and metas. If you want to get rid of the annotations and metas, you can simply create a new struct out of the MutableStructFields using ionStructOf(). (By "new struct", I'm referring to the user's intentions—you get a new instance of StructElement either way.)

What do you think? Do you prefer the lambda expression (Kotlin) and consumer-builder (Java) or the traditional builder syntax?

I prefer the former. But are you suggesting to add the update method in addition to mutableStructFields()?. I like mutableStructFields() because you can assign it to a variable and pass it around kind of like a StringBuilder. With update you are completing the whole process at that moment. I would propose to add both.

I'm fine with having both.

My initial comment was that we should leave out the toStruct() function since there's already ionStructOf(). I'm not strongly opposed to it—it's just my default opinion that if something seems redundant, we should probably leave it out. If you have a strong opinion that toStruct() should exist, and it should be a member (as opposed to e.g. an extension function), then let's keep it.

popematt · 2023-10-24T00:44:01Z

src/com/amazon/ionelement/impl/MutableStructFieldsImpl.kt

+        return fields[fieldName]?.firstOrNull()
+    }
+
+    override fun getAll(fieldName: String): Iterable<AnyElement> {


Suggestion—I suspect that users of the library might want to count the number of fields for a given name, so we may want to consider returning Collection<AnyElement> here instead. What do you think?

Good point, I will update.

Co-authored-by: Matthew Pope <[email protected]>

* Change `mutableStructFields` from property to method * Remove `copy` StructElement, method 6can be added later * Added `update` StructElement, method

Implementing MutableCollection instead of Iterator will allow extra functionality built into MutableCollection. To simplify implementing `iterator()` on MutableStructFields the internal backing field was also changed to MutableMap<String, MutableList<StructField>> at the expense of slightly more overhead because the underlying struct's field name is stored in the map key as well as value. This can be optimized in the future if desired.

src/com/amazon/ionelement/api/MutableStructFields.kt

popematt

Thanks! This is great.

Add Mutable View for Struct Elements

70894f0

Adds a MutableStructFields interface which represents an iterable sequence of mutable fields that can be used to construct a StructElement.

tjcsrc16 commented Oct 23, 2023

View reviewed changes

src/com/amazon/ionelement/impl/StructElementImpl.kt Show resolved Hide resolved

popematt reviewed Oct 25, 2023

View reviewed changes

tjcsrc16 and others added 6 commits November 2, 2023 09:19

Update src/com/amazon/ionelement/api/MutableStructFields.kt

cdbcc98

Co-authored-by: Matthew Pope <[email protected]>

Refactor MutableStructFields

a1d3f90

* Change `mutableStructFields` from property to method * Remove `copy` StructElement, method 6can be added later * Added `update` StructElement, method

Revert formatting change

56b71f3

Revert formatting change

35fa210

Add Missing Test Cases

8d00943

popematt reviewed Dec 7, 2023

View reviewed changes

src/com/amazon/ionelement/api/MutableStructFields.kt Outdated Show resolved Hide resolved

Update src/com/amazon/ionelement/api/MutableStructFields.kt

ecc1b95

popematt approved these changes Dec 7, 2023

View reviewed changes

popematt merged commit fbd44b4 into amazon-ion:master Dec 7, 2023
5 checks passed

tjcsrc16 deleted the mutable-struct-fields branch December 7, 2023 22:03

popematt mentioned this pull request Jun 27, 2024

Struct update dsl #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Mutable View for Struct Elements #84

Add Mutable View for Struct Elements #84

tjcsrc16 commented Oct 23, 2023

codecov bot commented Oct 23, 2023 •

edited

Loading

popematt left a comment

popematt Oct 23, 2023

tjcsrc16 Nov 2, 2023

tjcsrc16 Nov 2, 2023

popematt Nov 2, 2023

popematt Oct 23, 2023

tjcsrc16 Nov 2, 2023

popematt Oct 23, 2023

tjcsrc16 Nov 2, 2023

popematt Oct 23, 2023

tjcsrc16 Nov 2, 2023

popematt Oct 24, 2023

tjcsrc16 Nov 2, 2023

popematt Oct 24, 2023

popematt Oct 24, 2023

popematt Oct 24, 2023

tjcsrc16 Nov 2, 2023

popematt Nov 2, 2023

tjcsrc16 Nov 15, 2023 •

edited

Loading

popematt Nov 15, 2023

popematt Oct 24, 2023

tjcsrc16 Nov 2, 2023

popematt left a comment

		/** A mutable shallow copy of this struct's fields */
		public val mutableFields: MutableStructFields

Add Mutable View for Struct Elements #84

Add Mutable View for Struct Elements #84

Conversation

tjcsrc16 commented Oct 23, 2023

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

popematt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcsrc16 Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2023 •

edited

Loading

tjcsrc16 Nov 15, 2023 •

edited

Loading