roachpb: remove various (gogoproto.equal) options#56602
Merged
craig[bot] merged 6 commits intocockroachdb:masterfrom Nov 12, 2020
Merged
roachpb: remove various (gogoproto.equal) options#56602craig[bot] merged 6 commits intocockroachdb:masterfrom
(gogoproto.equal) options#56602craig[bot] merged 6 commits intocockroachdb:masterfrom
Conversation
We are planning to add an `errorspb.EncodedError` to `Error`, and if `Error` has `Equal()` codegen'ed for it, it needs to be present on `EncodedError` as well, which it isn't. Instead of putting it there, go the other way - there's generally no reason to have equality methods defined on everything under the sun, in fact they can be footguns. This first commit removes the ones that were in the way of deleting `Error.Equal`. More cleanups will follow for other error details. Release note: None
This removes the `(gogoproto.equal)` option from all ErrorDetails (and more generally everything in `errors.proto`). Looks like they were wholly unused. Release note: None
We had generated it on `RequestHeader`, which, by virtue of being embedded in all KV request protos, forced said protos to all generate it as well (as not doing so would otherwise expose the RequestHeader's Equal on the proto itself, which gogoproto rightly lints against). It turns out that RequestHeader doesn't need Equal, so we get to delete a lot of (generated) code, which is always a good thing. Release note: None
I removed all of the equal options from data.proto and added only those back for which there was a compelling reason. In investigating, I found a few in other proto files that could also be removed. Release note: None
It wasn't used outside of a few tests. Release note: None
We can compare it directly since we only ever wanted to compare it to zero. Release note: None
Member
|
This change is |
nvb
approved these changes
Nov 12, 2020
Contributor
nvb
left a comment
There was a problem hiding this comment.
I'm hoping this means that I can remove the EqOrdering vs. Equal confusion over in #56373.
Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 14 of 14 files at r4, 6 of 6 files at r5, 9 of 9 files at r6.
Reviewable status:complete! 1 of 0 LGTMs obtained
Member
Author
|
bors r=nvanbenschoten |
Contributor
|
This PR was included in a batch that was canceled, it will be automatically retried |
Contributor
|
Build succeeded: |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The first commit explains why some cleanup was necessary,
the others are the result of spending a little extra time
cleaning up "unnecessarily".
There are plenty of Equals left to clean up, but the returns
were diminishing. The important part is that when additions
to the KV API are made, nobody will be forced to add the
equaloption any more.Equalmethods