Closed jameslinjl closed 1 year ago
hey @bdferris ! any chance you can give this guy a review? i haven't had any luck finding other reviewers
cc @bdferris-v2
Getting close! Apologies for all the comments :)
Getting close! Apologies for all the comments :)
No need to apologize! Feedback is always appreciated :D
Apologies, I dropped offline for holiday vacations, but I am back now. This LGTM so I'm going to approve for merging.
That said, I'm going to add one last nit. Do with it what you will. The script name you chose update_protos.sh
is slightly weirdly named to me because in Protocol Buffer land, "protos" are usually the .proto
file itself or binary data encoded using that schema. Your script is updating the generated code bindings from the source proto file, not updating the proto file itself. So a more appropriate name might be update_bindings.sh
or update_generated_code.sh
or something similar. As I said, ultimately not that big a deal, but I move a lot of protos as a Googler, so it's triggering for me :)
@bdferris-v2 no problem! the file name change makes a lot of sense to me, so i included a change there. excited to get this merged in :)
Still LGTM. But one sad wrinkle. My review hasn't unblocked you for merging because it appears I don't have commit privileges on this repo :( I believe my old github account @bdferris has commit privileges, but I got locked out of that one a while back (don't lose your 2FA backup codes!). Normally, @isabelle-dr would fix this for us, but I think she's still out on vacation for the rest of the week.
Let me check to see if there is anyone who can get us unblocked in the meantime.
Good news. I got my privileges updated so you are now free to merge! Thanks for the contribution :)
@bdferris-v2 looks like the repo is configured so that I'm not allowed to click the button heh
Ah, well. I guess I can do the hard work of pressing the button.