support union marshal and unmarshal interface #522
Conversation
There was a problem hiding this comment.
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
codec_union.go:393
- Ensure that the type conversion via UnionUnmarshaller is safe; consider adding extra validation or error handling to prevent potential runtime panics if the target type does not fully implement the interface.
isUnionUnmarshaller := d.typ.Implements(reflect2.Type2(reflect.TypeFor[UnionUnmarshaller]()))
codec.go:41
- [nitpick] The use of an intermediate variable 'obj2' and its assignment to 'obj' for union unmarshalling could reduce code clarity; consider renaming 'obj2' or adding a clarifying comment to improve readability.
obj = &obj2
nrwiersma
left a comment
nrwiersma
left a comment
There was a problem hiding this comment.
Interesting idea. I think you are handling this way to high up in the structure. Likely this needs to be a codec like textMarshalerCodec.
I've tried to reimplement it in two codecs (one for Unmarshaller and one for Marshaller), but do note that this is the first time I've used the
Given the |
… during decode and remove redundant code
|
@nrwiersma it looks like the github action needs to be updated but changing CI/CD of another project feels wrong. Based on an earlier discussion lemurheavy/coveralls-public#1716 , the inputs (https://github.com/coverallsapp/github-action?tab=readme-ov-file#inputs) need to be adjusted to reflect the parallel builds - name: Coveralls
uses: coverallsapp/[email protected]
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
parallel: true
flag-name: go-${{ matrix.go-version }}
path-to-lcov: coverage.lcovI see that #524 has the same error |
|
thanks for the review @nrwiersma, I'll pick it up somewhere this week |
…interface' into feature/union-marshal-unmarshal-interface
|
@nrwiersma have you had time to take a look at the changes? |
nrwiersma
left a comment
nrwiersma
left a comment
There was a problem hiding this comment.
I will look at the test coverage tonight, a couple nitpicks
|
Test coverage looks good to me. Once the 2 nitpicks are resolved I think we can merge. |
|
Updated branch and implemented comments |
2 participants
Goal of this PR
Currently when implementing (not strictly nullable) unions, one has to fall back to either
anyormap[string]anywhile from a readability perspective it would be nice to see the actual possible Go types. This also makes the consumption of a received message easier as no casting to a type has to performed anymore and likewise creating objects to serialise is less error prone because no invalid type can be used (assuming that the struct is implemented correctly).I've updated the README rather verbose, looking for feedback on the PR and the updated README
Fixes #
n/a
How did I test it?
The example in the README is tested from in-memory > marshal to avro bytes > unmarshal from avro bytes > same as before