deps: move from github.com/golang/protobuf to google.golang.org/protobuf/proto #6961
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #6961 +/- ##
==========================================
- Coverage 83.71% 82.34% -1.38%
==========================================
Files 287 296 +9
Lines 30926 31471 +545
==========================================
+ Hits 25889 25914 +25
- Misses 3972 4487 +515
- Partials 1065 1070 +5
|
internal/pretty/pretty.go
Outdated
| @@ -36,17 +35,17 @@ const jsonIndent = " " | |||
| // If marshal fails, it falls back to fmt.Sprintf("%+v"). | |||
| func ToJSON(e any) string { | |||
| switch ee := e.(type) { | |||
| case protov1.Message: | |||
| case protoadapt.MessageV1: | |||
There was a problem hiding this comment.
nit: can we combine some of this now from below case: protoadapt.MessageV2?
There was a problem hiding this comment.
+1; it would be nice to unify these two codepaths completely, but feel free to disregard if you don't want to do it.
I.e.
if ee, ok := e.(protoadapt.MessageV1); ok {
e = protoadapt.MessageV2Of(e)
}
switch ee := e.(type) {
// no case for MessageV1 now
...I don't know why the existing code sets different MarshalOptions but I highly suspect it's an oversight caused by having two codepaths that do essentially the same thing.
| @@ -9,7 +9,6 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| github.com/golang/protobuf v1.5.3 // indirect | |||
There was a problem hiding this comment.
How are these deps changing without any changes under this sub module?
There was a problem hiding this comment.
This is an indirect dep, so it changes because the grpc deps change.
I don't believe this should be changed. It tests that we will work with old generated code; we'd like to still confirm that fact in our tests. |
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
@Clement-Jean -- friendly ping here, didnt want the bot to mark this stale |
|
@arvindbr8 could you take a look at the comment on your suggestions? Not entirely sure about how to proceed |
|
Apologies for the delay -- @Clement-Jean. I have been busy with some internal priorities. Taking a look now. |
|
Also it is weird that I'm not able to see your reply on my comment. however there was a email notification for it
you can ignore this comment. That was only a nit to combine the logic in the both the switch cases. Im not too inclined for it Please let me know if you have other questions. |
|
@arvindbr8 so basically right now we are blocked at this point. We need to wait for @dfawley rewrite of channelz. Once this is done, we can go further and do the change in |
|
Ah I see.. I would prefer if we could decouple those changes. You can ignore my comment there. I will track that suggestion in Doug's PR. |
|
Approved. Thanks for the PR. We need 1 more person from the team to take a look at it. |
| @@ -9,7 +9,6 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| github.com/golang/protobuf v1.5.3 // indirect | |||
There was a problem hiding this comment.
This is an indirect dep, so it changes because the grpc deps change.
internal/pretty/pretty.go
Outdated
| @@ -36,17 +35,17 @@ const jsonIndent = " " | |||
| // If marshal fails, it falls back to fmt.Sprintf("%+v"). | |||
| func ToJSON(e any) string { | |||
| switch ee := e.(type) { | |||
| case protov1.Message: | |||
| case protoadapt.MessageV1: | |||
There was a problem hiding this comment.
+1; it would be nice to unify these two codepaths completely, but feel free to disregard if you don't want to do it.
I.e.
if ee, ok := e.(protoadapt.MessageV1); ok {
e = protoadapt.MessageV2Of(e)
}
switch ee := e.(type) {
// no case for MessageV1 now
...I don't know why the existing code sets different MarshalOptions but I highly suspect it's an oversight caused by having two codepaths that do essentially the same thing.
|
Thanks for the PR! I went ahead and made some minor cleanup edits to speed things up; hope that's okay. :) |
@dfawley this is the continuation of #6919
I'm still unsure about the reflection package. I read the README there and it says that we shouldn't regenerate the testv3.go file. I see two solutions, either we manually edit the generated file (this was previously done) or we regenerate with an older version of protoc. What do you think?
RELEASE NOTES: none