Code refactoring

[dmlambea]

Hi, @actgardner! Just wanted to tell you I'm doing some experiments in the branch "wip-code-refactoring" of my fork, based in the PR #127 I opened some days ago.

In my branch I tried to reduce duplicated code by applying an initial refactoring of the base types, which led to removal of the resolver package and some other minor changes in the templates. I am currently testing that branch and I've found the code a bit easier to maintain, as well as the generated Go code easier to deal with (because of #127 is handling optional unions properly). See this example:

AVRO type:

{
    "name": "OptionalUnionTestRecord",
    "type": "record",
    "fields": [
        {
            "name": "nonOption",
            "type": {
                "name": "UserRecord",
                "type": "record",
                "fields": [
                    {
                        "name": "username",
                        "type": "string"
                    },
                    {
                        "name": "userID",
                        "type": "long"
                    }
                ]
            }
        },
        {
            "name": "option",
            "type": [
                "null",
                "string",
                "int"
            ]
        }
    ]
}

Generated code snippets:

// Snippet from optional_union_test_record.go
...
type OptionalUnionTestRecord struct { 
  NonOption UserRecord           // Notice the non-pointer type
  Option    *UnionNullStringInt
}
...

// Snippet from union_null_string_int.go
...
type UnionNullStringIntTypeEnum int

const (
  UnionNullStringIntTypeEnumString UnionNullStringIntTypeEnum = 1
  UnionNullStringIntTypeEnumInt UnionNullStringIntTypeEnum = 2
)

type UnionNullStringInt struct { 
  String    string
  Int       int32
  UnionType UnionNullStringIntTypeEnum
}
...

Using these types work as one might expect:

...
// Please notice the nil Option field
rec := avro.OptionalUnionTestRecord{
  NonOption: avro.UserRecord{
    Username: "user",
    UserID:   42,
  },
}
rec.Serialize(w)  // Serializes the record using the NULL union type for Option field

The non-null types work as usual:

rec := avro.OptionalUnionTestRecord{
  ...
  Option: &avro.UnionNullStringInt{
    UnionType: avro.UnionNullStringIntTypeEnumInt,
    Int: 123,
  },
}
rec.Serialize(w)  // Serializes the record using the INT union type for Option field

While the refactored code works for me, I understand it might be a bit too much for a PR. But if you like the way my experiment is going, I would be happy to open a PR.

[actgardner]

Hey @dmlambea, sorry for the slow response. Your current PR and this proposed one both sound great. One thing I'd ask in a big refactor is if you can split the changes into individual PRs and/or commits that have a specfic purpose. That way it would be easier for me to read and give feedback on specific changes while understanding the intent behind them, versus getting one large PR and having to infer what's happening. Thanks for all your work on this!

[dmlambea]

Hi, @actgardner, I'm sorry for not contacting you before. This COVID-19 thing is putting a lot of pressure in the gov's IT dept., and I had been focused in other priority tasks for the past days. Due to this, the works in my WIP branch has suffered a slowdown.

I am glad that you liked my proposal, thank you very much, really! Sure, I can work with you to deliver the changes in the way you find it easier. But I still have pending changes I believe you will want to include in the major version.

The most relevant part is the one relative to the proposal described in then long-forgotten #40, which I wanted to try and implement, given the fact that the optional unions are just one step behind achieving the proposal's goal. I think I've got something, but the amount of changes for adapting and testing are requiring me a lot of time, and I think it will take a little bit more of time to be PR-ready. I use gogen-avro a lot and I'd like to commit well written, tested code.

If you can wait for the final changes to be ready, I'm sure you will find it well worth it!