Add analyzer to enforce usage of VT versions of marshalling and unmarshalling #2043
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
github-actions
bot
added
area/datastore
tstirrat15
force-pushed
the
2036-enforce-usage-of-VT
branch
from
a1aa797
to
f4deb83
Compare
|
Had a conversation with @josephschorr where it'd be nice to have a sort of snapshot test that serializes a schema out to protobuf bytes using both VT and non-VT, and then the test deserializes using the opposite and then asserts that they agree, with the idea being that it's a snapshot test that will alert us if our serialization or deserialization changes meaningfully. |
jzelinskie
reviewed
Aug 27, 2024
| "golang.org/x/tools/go/ast/inspector" | ||
| ) | ||
|
|
||
| func sliceMap(s []string, f func(value string) string) []string { |
There was a problem hiding this comment.
I think there's a function for this in the lo library we import some places
There was a problem hiding this comment.
Is it named the same? I copied this code from an existing analyzer.
There was a problem hiding this comment.
vroldanbet
previously approved these changes
Aug 29, 2024
| "golang.org/x/tools/go/ast/inspector" | ||
| ) | ||
|
|
||
| func sliceMap(s []string, f func(value string) string) []string { |
There was a problem hiding this comment.
vroldanbet
previously approved these changes
Aug 30, 2024
tstirrat15
force-pushed
the
2036-enforce-usage-of-VT
branch
from
33b9ec0
to
62d23b2
Compare
vroldanbet
previously approved these changes
Aug 30, 2024
tstirrat15
force-pushed
the
2036-enforce-usage-of-VT
branch
from
a842708
to
3043ca3
Compare
josephschorr
approved these changes
Aug 30, 2024
tstirrat15
force-pushed
the
2036-enforce-usage-of-VT
branch
from
ebffc47
to
3950fbf
Compare
tstirrat15
force-pushed
the
2036-enforce-usage-of-VT
branch
from
3950fbf
to
0dd3667
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
Calling
proto.Marshalrequires calling reflection code, where theVTmethods are called directly and are more efficient. This adds a linter that enforces that.Note
The exclusions ended up being fairly broad because of the size of the excluded packages. One thing that was discussed was implementing
// nolintcomments for this analyzer, but that would require either writing that manually into the analyzer or making use of a package that provides that functionality, and I wasn't sure whether we wanted to take that on right now.Changes
Testing
Review the code. See that lints go green.