Remove inline typeconvert when logging some thrift typedefs #464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Codecov Report
@@ Coverage Diff @@
## dev #464 +/- ##
==========================================
+ Coverage 78.96% 79.00% +0.03%
==========================================
Files 125 125
Lines 15981 16048 +67
==========================================
+ Hits 12620 12678 +58
- Misses 2056 2060 +4
- Partials 1305 1310 +5
Continue to review full report at Codecov.
|
abhinav
reviewed
Feb 11, 2021
gen/zap.go
Outdated
| // zapTypedefGenerateMarshaler is similar to zapMarshaler but always converts a typedef | ||
| // to its root value. This should be used when generating the code for the Zap | ||
| // marshal implementation of the typedef. | ||
| func (z *zapGenerator) zapTypedefGenerateMarshaler(g Generator, spec compile.TypeSpec, fieldValue string) (string, error) { |
There was a problem hiding this comment.
I'm not sure why we need this separate function exposed in the template? Looks
like zapMarshaler can (and does) already do this if spec is a
compile.TypedefSpec.
There was a problem hiding this comment.
This zapMarshaler is used in the template that generates MarshalLogObject/MarshalLogArray for typedef, as well as in the template for other object's MarshalLog... function if they have typedef for a field. In the field case you want to not convert to the underlying go type, in the other one you need to typeconvert to the underlying to avoid the typedef's generated MarshalLog... to call itself.
To avoid adding adding an extra parameter to zapMarshaler (something like forceTypedefToRoot bool), I made/exported a new function.
robbertvanginkel
force-pushed
the
robbert/direct-deps
branch
from
fa663f4
to
46a6854
Compare
#366 added fast zap logging by generating extra `MarshalLog...` methods on generated types. For typedefs in thrift, other types that log them will cast to the underlying type in their `MarshalLog...` method, causing the generated code to need to import multiple go packages generated from both direct and indirect thrift dependencies. This should not be necessary, as typedefs of complex types (anything that cannot be mapped directly to a go stdlb type) get their own `MarshalLog...` methods, which cast to underlying type when necessary. This PR changes the logging for typedef'ed fields to: - cast to the underlying type inline when the underlying thrift type directly maps to a go type. - defer to the typedef's generated `MarshalLog...` method otherwise. This results in having more predictable importpaths (eg knowing that one thrift import will lead to one go import in generated code) makes implementing good bazel rules for thriftrw easier. See internal issue GO-362 for a more elaborate example of how the current behaviour complicates our Bazel setup.
robbertvanginkel
force-pushed
the
robbert/direct-deps
branch
from
46a6854
to
bbdb77f
Compare
AllenLuUber
reviewed
Feb 17, 2021
AllenLuUber
reviewed
Feb 17, 2021
AllenLuUber
approved these changes
Feb 17, 2021
abhinav
reviewed
Feb 18, 2021
There was a problem hiding this comment.
Mostly LGTM.
To address coverage drop, check out gen/quick_test.go. We need to add newly declared types there.
Co-authored-by: Abhinav Gupta <abg@uber.com>
abhinav
approved these changes
Feb 18, 2021
r-hang
pushed a commit
that referenced
this pull request
Feb 18, 2021
#366 added fast zap logging by generating extra `MarshalLog...` methods on generated types. For typedefs in thrift, other types that log them will cast to the underlying type in their `MarshalLog...` method, causing the generated code to need to import multiple go packages generated from both direct and indirect thrift dependencies. This should not be necessary, as typedefs of complex types (anything that cannot be mapped directly to a go stdlib type) get their own `MarshalLog...` methods, which cast to underlying type when necessary. This PR changes the logging for typedef'ed fields to: - cast to the underlying type inline when the underlying thrift type directly maps to a go type. - defer to the typedef's generated `MarshalLog...` method otherwise. This results in having more predictable importpaths (eg knowing that one thrift import will lead to one go import in generated code) makes implementing good bazel rules for thriftrw easier. See internal issue GO-362 for a more elaborate example of how the current behaviour complicates our Bazel setup. Co-authored-by: Abhinav Gupta <abg@uber.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#366 added fast zap logging
by generating extra
MarshalLog...methods on generated types. Fortypedefs in thrift, other types that log them will cast to the
underlying type in their
MarshalLog...method, causing the generatedcode to need to import multiple go packages generated from both direct
and indirect thrift dependencies.
This should not be necessary, as typedefs of complex types (anything
that cannot be mapped directly to a go stdlb type) get their own
MarshalLog...methods, which cast to underlying type when necessary.This PR changes the logging for typedef'ed fields to:
directly maps to a go type.
MarshalLog...method otherwise.This results in having more predictable importpaths (eg knowing that one
thrift import will lead to one go import in generated code) makes
implementing good bazel rules for thriftrw easier.
See internal issue GO-362 for a more elaborate example of how the
current behaviour complicates our Bazel setup.