3 ms·

>> Our RPCs are named as GetFoo, not DetermineFoo >This is a nit. I'd honestly probably ignore it. Congratulations, until the end of time your team now has to

>> Our RPCs are named as GetFoo, not DetermineFoo >This is a nit. I'd honestly probably ignore it. Congratulations, until the end of time your team now has to remember multiple naming conventions, and when they want to do a code search for the RPC that retrieves foo, if they search for "GetFoo" because all the other RPCs are "GetBar", "GetBaz", and "GetTaco", they'll find no results and be confused. "The team" is not a static thing, it is an evolving team-of-Theseus. Consistent naming and coding styles make it easier for the team, until the end of time, to read and search code. These "nits" are productivity multipliers. As other reviewers have called out, I am a fan of "LGTM with comments", but any teammate who starts ignoring comments loses the privilege of receiving LGTM with comments from me and gets to wait until I actually review a version of the code that is acceptable to the team.

withinboredom 3y ago
> until the end of time your team now has to remember multiple naming conventions Do they not have PR's where you are from? If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa. The code is mutable. It doesn't have to be perfect on the first iteration. I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.
- Arainach 3y ago
  The code is mutable, but the cost to fix it during the original code review is WAY lower. Once you land RPC DescribeFoo, I have to: 1) Define a second parallel RPC GetFoo 2) Wait for it to roll out to prod 3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover 4) Wait for all of those to roll out 5) Clean up all the callers 6) Clean up the DescribeFoo RPC .....or you could just fix it in code review with half an hour of work.
  - withinboredom 3y ago
    I thought it was DetermineFoo? Anyway. To me, DetermineFoo doesn't sound like a getter and seems like it might even possibly mutate some states. So, I suspect creating a GetFoo would be orthogonal to deprecating DetermineFoo. This means DetermineFoo might only Get a Foo by side-effect. That's why I would probably ignore it; simply because DetermineFoo might be a better name, in my opinion. New behaviors occur all the time in software, but it doesn't mean we need to be rigid with naming things.

withinboredom 3y ago

> until the end of time your team now has to remember multiple naming conventions Do they not have PR's where you are from? If someone is that annoyed by it, just open a PR. Just because I don't find it annoying, doesn't mean someone else won't. And vice-versa. The code is mutable. It doesn't have to be perfect on the first iteration. I will say this though: if you want me to do your nits, you also have to do mine. It's a two-way street when it comes to nits. If you don't do my nits, then I won't do yours. It's pretty simple. If you start withholding approvals because I stop doing your nits, then yay ... politics, I guess.

Arainach 3y ago
The code is mutable, but the cost to fix it during the original code review is WAY lower. Once you land RPC DescribeFoo, I have to: 1) Define a second parallel RPC GetFoo 2) Wait for it to roll out to prod 3) Update all callers to call GetFoo, probably controlled with an experiment for safe rollover 4) Wait for all of those to roll out 5) Clean up all the callers 6) Clean up the DescribeFoo RPC .....or you could just fix it in code review with half an hour of work.
- withinboredom 3y ago
  I thought it was DetermineFoo? Anyway. To me, DetermineFoo doesn't sound like a getter and seems like it might even possibly mutate some states. So, I suspect creating a GetFoo would be orthogonal to deprecating DetermineFoo. This means DetermineFoo might only Get a Foo by side-effect. That's why I would probably ignore it; simply because DetermineFoo might be a better name, in my opinion. New behaviors occur all the time in software, but it doesn't mean we need to be rigid with naming things.