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
gen: Generate getters for optional primitives #325
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😻 I'm very, very happy that we can make it easier to migrate from Apache Thrift.
gen/field.go
Outdated
<range .Fields> | ||
<$fname := goName .> | ||
<if and (not .Required) (isPrimitiveType .Type)> | ||
func (<$v> *<$name>) Get<$fname>() (<$o> <typeReference .Type>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice - named return makes the generated code simpler.
gen/field.go
Outdated
<$name := .Name> | ||
<range .Fields> | ||
<$fname := goName .> | ||
<if and (not .Required) (isPrimitiveType .Type)> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does generating the same type of accessors for non-primitive types introduce much extra complexity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but you don't gain anything by doing it. Non-primitive types are always passed around as pointers so the accessor will simply return the value of the field. x.Foo
will be no different from x.GetFoo()
, so users have no reason to use GetFoo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense.
The majority use case for accessing values of optional primitives is to dereference it as-is or use the default/zero value if not. This modifies code generation to include accessor functions for optional primitive fields which return non-pointer versions of those fields. So instead of, var foo string if s.Foo != nil { foo = *s.Foo } doSomething(foo) Users can do, doSomething(s.GetFoo()) This should also ease migration for users coming from Apache Thrift. Resolves #295
bc16620
to
fd608f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is overall a win. Some services may fail to compile after an idl fetch
in the unlikely event of an existing conflict. I think it’s a worthwhile risk.
|
||
struct AccessorConflict { | ||
1: optional string name | ||
2: optional string get_name (go.name = "GetName2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially a breaking change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is potentially breaking but for unlikely conflicts, we've taken the policy of suggesting "use go.name
in case of conflict" rather than cutting a 2.0.
From a quick look at all IDLs in our system, I don't see anything this would actually cause conflicts for. When people use "getFoo" in the request struct, "foo" is usually an optional field in the response struct.
The majority use case for accessing values of optional primitives is to
dereference it as-is or use the default/zero value if not.
This modifies code generation to include accessor functions for optional
primitive fields which return non-pointer versions of those fields.
So instead of,
Users can do,
This should also ease migration for users coming from Apache Thrift.
Resolves #295
Depends on #324