amazon-ion / ion-cli

Apache License 2.0
35 stars 15 forks source link

Adds changes for optional/required fields in Java code generation #148

Closed desaikd closed 1 month ago

desaikd commented 1 month ago

Issue #, if available:

Description of changes:

This PR works on adding support for optional/required fields in a struct. (This PR only adds support for optional/required fields in Java, Rust support is still pending).

List of changes:

Generated Java code:

Generated Java code for given test schema files can be found here. (Only the files that has changes in schema are added, other files remain same)

Test:


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

desaikd commented 1 month ago

Latest changes adds changes to the generated code as following:

Generated Java code can be found here.

List of changes:

desaikd commented 1 month ago

So, I took a closer look at some of the generated code, and there's a few things that I'd like to mention.

    public void setC(
        java.util.Optional<Boolean> d,
        java.util.Optional<java.util.ArrayList<Integer>> e
    ) {
        org.example.NestedStruct.NestedType1 c = new org.example.NestedStruct.NestedType1();
        c.setE(e);
        c.setD(d);
        this.c = java.util.Optional.of(c);
        return;
    }
* This is going to be really inefficient having e.g. `Optional<ArrayList<Integer>>`.

* I think I disagree with using `Optional`, but I'm not going to die on that hill. If we are going to be using `Optional`, there are `Optional*` types for some primitives that will be a bit of an improvement.

I think you are looking at the older version of generated code before suggested changes. Here's the new one : https://gist.github.com/desaikd/91c238e15852b60a0cc3e3fc1f106261 (I have also added comment about these changes here)

* The most surprising this about this to me is that `setC()` doesn't take a `NestedStruct.NestedType1`. Instead, it takes the components of that type and constructs one in the setter. That's going to be surprising to most Java developers, I think.

I think I intended to hide the details about the intermediate types which are generated as nested classes. It also aligns with what the actual schema expects, for example in this case field c expects two fields d and e. So all the places where a NestedTypeX is expected as an argument I have used its properties as argument. This would be easier once we have a $code_gen_name for these types and user will have a context to what this name refers to.

popematt commented 1 month ago

I think I intended to hide the details about the intermediate types which are generated as nested classes. It also aligns with what the actual schema expects, for example in this case field c expects two fields d and e. So all the places where a NestedTypeX is expected as an argument I have used its properties as argument.

...except that even as the setter attempts to hide NestedType1, the getter returns an instance of NestedType1. We can't hide it, and the asymmetry makes it all the more surprising.

This would be easier once we have a $code_gen_name for these types and user will have a context to what this name refers to.

Even without a $code_gen_name field, we can use the field name as the class name—i.e C in this case. It's weird when you have single character names, but if the field name was something like coordinates or ratings, then generating a Coordinates class or a Ratings class is actually a really sensible thing to do.

If the change to try to hide nested types in the setters was introduced in this PR, then I think it needs to be undone before merging this PR. If it was done in a prior PR, then lets create an issue to go back and remove it before we begin asking anyone to start using it.

desaikd commented 1 month ago

...except that even as the setter attempts to hide NestedType1, the getter returns an instance of NestedType1. We can't hide it, and the asymmetry makes it all the more surprising.

True. I think making both the getters and setters return NestedTypeX in that case makes sense.

If the change to try to hide nested types in the setters was introduced in this PR, then I think it needs to be undone before merging this PR. If it was done in a prior PR, then lets create an issue to go back and remove it before we begin asking anyone to start using it.

This was added way back when the intermediate types were added to be generated as nested types and isn't introduced in this PR. I have created an issue for this here: https://github.com/amazon-ion/ion-cli/issues/152