[swiftcoder]

> In a higher trust environment, PR review is a helpful thing you usually desire, not dread

Respectfully, in a high-trust environment, feedback should be delivered well before the PR stage. If you've let someone write a whole bunch of code without having a shared understanding of how the solution should work, you may have earlier process issues that PRs are papering over

[rplnt]

You cannot deliver feedback on something that doesn't exist. If you mean a review in the style of "all of this is wrong and needs to be rewritten differently" then yes, that's something to be discussed beforehand. But I don't imagine this is what people think of when discussing a review.

[jonahx]

Depends on how PRs function within teams. For some, the PR is a lightweight thing that is the preferred method of communication. It sounds like you are imagining a case where face to face communication, or communication over chat, is preferred for early stages, with the PR being a nearly final artifact. But it doesn't have to work like that.

[swiftcoder]

I think that's a valuable point. Especially as LLMs bring the cost of prototyping down (and reduce emotional investment in code written), it may be more viable to use PRs as proposals/sketches of a solution.

With human reviewers, I find that by the time someone has churned out enough of a solution to post a PR, they are already quite invested in specifics of the solution, and it makes it emotionally costly (to both author and reviewer) when someone says "hey, I'm not a fan of this whole approach, lets start over and do it this other way"

[necovek]

I have seen many a PR where it is obvious it is an exploratory work: eg. figuring out how to use an external dependency that is imperfectly or incorrectly documented, etc. (You can claim this should be done ahead of time, but experience tells me you need to code it to learn it)

The emotional toll there is real, but this is exactly the moment when you expose the knowledge of that external dependency to the unbiased party that is the reviewer.

I like combining approvals to satisfy the urge for completion and closure, with a request for fast-follow refactor to better match the newly discovered model of interaction. (The worst code review experience I have seen is when a reviewer accepts it as-is and does a fast follow refactor themselves, depriving the author of the opportunity to learn and remain an expert in that area)

[necovek]

A discussion ahead of the implementation can also bias the two parties to that discussion and have them overlook the same implementation issue: many things you only understand once you start implementing.

If you have these parties review each other's code, I agree that rarely brings much value.

I think the best way to understand our experience with reviews is to stop and say: in a few sentences, what do you expect out of a quality code review? (sounds like nothing in your case, but I am curious)

[swiftcoder]

> in a few sentences, what do you expect out of a quality code review? (sounds like nothing in your case, but I am curious)

From my perspective, there are three sorts of PRs:

- One is very close to the final form of a particular change, and any feedback you get at that late stage is indicative of holes in your process.

- Another is one where someone throws something up and says "hey, this is an experiment, can I get feedback on the approach". This is great, the parameters are clear, not much to say about these.

- The 3rd sort is someone making a trivial 5-line patch to a makefile/cargo.toml/github workflow/etc. These add basically no value to anyone.

Of those only the 2nd type really brings much value, and those are the ones that folks would keep posting even if you didn't require PRs (since they have an actual question, or a cool thing to show off).

I'll also note that this only really negatively impacts small remote teams, because on a sufficiently large, co-located team, you just ask your buddy one desk over to rubber stamp all the trivial commits...

[necovek]

On the first category, what is a process you use which has no "holes" in it?

Does everybody produce completely readable, tested code every time? Perhaps that's just "style" to you when it is "maintainability" to me?

[swiftcoder]

> Does everybody produce completely readable, tested code every time?

Do your coworkers not reliably produce readable, tested code?

That's kind of the minimum bar for a software engineer in my book

[necovek]

I like to invert that to: do I produce code I am perfectly happy with in regards to readability and maintainability or would I benefit from another pair of eyes?

Every question I get (when my code is reviewed) is a signal that code could be more self-explanatory, unless it is a complex algorithm itself, and that my — by now deep — exposure to the problem is keeping me misguided about what is and isn't "obvious" or "clear". A reviewer can take a step back and help ensure both them and I will be able to easily grasp the same code 3 or 24 months later.

Note that one of the best advice I got early in my career about doing a good code review is that you "just" need to ask good questions: the point is not for a reviewer to show how much smarter they are, but for both to develop a shared understanding and ensure code can be interpreted as quickly as possible.

[IshKebab]

Depends on the change. Certainly most PRs don't need feedback before the PR is ready - the task is too obvious, and there's little to feed back on before there's any code.

For bigger changes, of course you need feedback on designs. But that could easily be in the form of draft PRs.

I definitely would push back on anything that required feedback before PRs. That's way too much process. Just going to slow you down for no benefit.

[teiferer]

Agreed. But those things are not mutually exclusive.

[jvanderbot]

Agree. All the subtleties of how a high trust environment work are hard to enumerate