3 ms·

Your null implementation did help put it into context to some extent. Even then, in general, the API surface sufficient for a null implementation may not be eno

by fl0ki 3y ago

Your null implementation did help put it into context to some extent. Even then, in general, the API surface sufficient for a null implementation may not be enough for some real implementations. What if at least one real implementation ended up needing another parameter, and making that parameter available required a cascade of changes through several other functions and types? If that's what happened, the second change would have to carry that weight, and the changes combined required more review overall than one change which had the final API from the start. I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Your reviewer probably trusted you to do that, or at least assumed it was in your interests to do that. But if that wasn't assumed or asserted, the reviewer could rightly ask how you know the first version is the right design based only on the null version. I've definitely seen people come up with an architecture, API, or even network protocol based on what they hope will work without testing it on any real implementation yet. Just one detail from the real implementation can undermine the whole idea, scrapping any dev and review work done up to that point. Your change didn't have that problem, but if we're recommending principles for others then it's only fair to say this can be a problem. I don't ask for a review for any design I haven't already fleshed out to a working solution, and even then, what works for one real consumer may not generalize well enough to other real consumers. But I consider one the bare minimum, and I make sure reviewers know what that one real implementation is as context for review, even if it does happen to be in a separate change or even separate repo. At an extreme, it's like how you don't post an RFC that imagines a neat protocol that could exist, you post an RFC describing a protocol that has already been implemented and can now be generalized for broader interoperability. An internal API surface is the other extreme because it is much easier to evolve later, and a library API surface is somewhere in between, but all of them benefit from demonstrating real-world implementations of the abstract interface.

munksbeer 3y ago
>I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Yes. But not only that, just wiring in the null implementation required several choices. I wanted to know that reviewers would focus on that important detail and it not get drowned out by a review of business logic. Larger PRs often have large parts of the code rubber stamped through sheer wariness of the reviewers. Like it or not, that happens. And that way design choices are approved that should not have. I see it all the time. You can argue that is the fault of the review system, but if it just constantly happens, how do you fix it? Keep banging at people to review properly? They're people, they're goin to take a path of least resistance, that just happens. If it were up to me I'd put in a CI failure step for PRs over 500 lines. You could override that step if people deemed it reasonable (eg. large search and replace PRs), but it would have to be used judiciously. I just generally find people who raise large PRs don't respect the reviewer enough to make the effort. So I often leave those PRs sitting, but I know someone is just going to come along and rubber stamp them.

by fl0ki 3y ago

munksbeer 3y ago

>I assume the way you wrote it is that you already had the real version ready so you knew the API was sufficient even when you only sent the null version for review. Yes. But not only that, just wiring in the null implementation required several choices. I wanted to know that reviewers would focus on that important detail and it not get drowned out by a review of business logic. Larger PRs often have large parts of the code rubber stamped through sheer wariness of the reviewers. Like it or not, that happens. And that way design choices are approved that should not have. I see it all the time. You can argue that is the fault of the review system, but if it just constantly happens, how do you fix it? Keep banging at people to review properly? They're people, they're goin to take a path of least resistance, that just happens. If it were up to me I'd put in a CI failure step for PRs over 500 lines. You could override that step if people deemed it reasonable (eg. large search and replace PRs), but it would have to be used judiciously. I just generally find people who raise large PRs don't respect the reviewer enough to make the effort. So I often leave those PRs sitting, but I know someone is just going to come along and rubber stamp them.