Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support custom logical types #429

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

Emptyless
Copy link
Contributor

proposal to support custom logical types (implementing TextMarshaller) when generating structs

@marcowork
Copy link

+1

@nrwiersma
Copy link
Member

Can you explain the issue you are trying to solve? Logical types are specified in the spec.

@Emptyless
Copy link
Contributor Author

Emptyless commented Aug 9, 2024

@nrwiersma the spec details a set of "official" logical types that most implementations support, it is however allowed to define your own custom LogicalType's that are marshallable by their base type, this has been applied in e.g. Java [1] and Python [2].

But this PR also allows you to register your own type for a particular "official" LogicalType, e.g. the UUID type. The UUID type is backed by a string but a string can be anything. By registering a UUID compliant Go type, you get added type safety when going back and forth between Avro. I've included this as an example in the tests

@Emptyless
Copy link
Contributor Author

fixed linting errors

Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good, just a couple nitpicks.

gen/gen.go Outdated
func WithLogicalType(logicalType LogicalType) OptsFunc {
return func(g *Generator) {
if g.logicalTypes == nil {
g.logicalTypes = make(map[avro.LogicalType]LogicalType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly more conventional.

Suggested change
g.logicalTypes = make(map[avro.LogicalType]LogicalType)
g.logicalTypes = map[avro.LogicalType]LogicalType{}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applied

gen/gen_test.go Outdated
Comment on lines 190 to 206
tests := map[string]struct {
config gen.Config
fileName string
}{
"without custom logical types": {
config: gen.Config{PackageName: "Something"},
fileName: "testdata/golden.go",
},
"with logical types": {
config: gen.Config{PackageName: "Something", LogicalTypes: []gen.LogicalType{{
Name: "uuid",
Typ: "uuid.UUID",
ThirdPartyImport: "github.com/google/uuid",
}}},
fileName: "testdata/golden_logicaltype.go",
},
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer this split into 2 tests. There are other golden tests, each in their own tests, so this does not really match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separated tests into their own

@Emptyless
Copy link
Contributor Author

resaonable suggestions, applied the fixes. make lint also mentioned the _, _ = fmt.Fprintln(stdout, schema) line so applied so that the pipeline won't break on it

Copy link
Member

@nrwiersma nrwiersma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉 Thanks for the contribution

@nrwiersma nrwiersma merged commit 51c1d35 into hamba:main Aug 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants