On Code Review

This implies the need for smaller pull requests maybe?

If work is broken into smaller chunks, addressing peer feedback isn't as costly, because there's time to back out of poor decisions if the peers get alerted to them early on.

Changing the course once the implementer has indeed invested lots of time and effort into a flawed solution is ineffective, true; but that might just mean the feedback was asked for too late.

"If you want to enforce standards, automate it"

Agreed, as long as you're thinking standards as in indentation, naming rules etc. All the simple stuff can be done with code inspections, and employing humans to enforce these "mechanical" standards is a waste of time. No argument there!

However, there are higher-level standards that can't really be automated though, or at least not effectively. Eg. various architectural conventions and the like.

"If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor"

Well, I can see how it could work in a small, lean project... but in some multi module behemoth with millions of LOC and history spanning back years?

"The code was already written. Time/money spent" rings much more strongly in that scenario that in reference to "regular" code review.

I'm 100% on-board with this. However, I understand were OP's mindset comes from. A few years ago I was working at a development shop in a fairly large project, where we had little to no automated tests. We frequently had PRs that would break main features. At some point developers were required to perform smoke tests alongside code reviews.

This makes me feel like perhaps pull requests shouldn't always pull/merge the changes directly, but instead there should be something like the GitLab "Start a review" functionality, where you can queue up code comments, before submitting all of them at once, but for pull requests.

As a Jira analogy, imagine the totality of the changes that you want to merge as an issue, but the individual steps/smaller requests as sub-tasks. The entire task would only be considered done after all of the sub-tasks are done, similarly, the changes should only be merged after all of the sub-requests have been successfully reviewed. Thus, development could happen more iteratively, with feedback sooner.

Otherwise you end up with something like the following being separate requests:

  - FEATURE-123 add data models for the feature
  - FEATURE-123 add service logic for the feature
  - FEATURE-123 add logging and validations for the feature
  - FEATURE-123 add unit tests for the feature
  - FEATURE-123 add REST endpoints for the feature
  - FEATURE-123 add front end code to forms
  - FEATURE-123 add front end logic to use the REST endpoints
  - FEATURE-123 add integration tests for headless browser

To me, all of this seems like a problem that stems from developers viewing code review as something to only be done at the moment when you want to merge the feature to your main branch. Even the tooling that we use is built with this in mind. It feels like there are better alternatives.

EDIT: So, you'd need something like the following:

  - (WIP, 3/8 reviewed) FEATURE-123 add new feature              // feature-123 branch, cannot be merged into main yet, review the below first
    - (reviewed) add data models for the feature                 // feature-123-data-models branch, will be merged into previous
    - (reviewed) add service logic for the feature               // feature-123-service-logic branch, will be merged into previous
    - (reviewed) add logging and validations for the feature     // feature-123-logging-and-validations branch, will be merged into previous
    - (in review, 9/13 comments resolved) add unit tests         // feature-123-unit-tests branch, will be merged into previous
    - (registered) add REST endpoints for the feature            // feature-123-rest-endpoints branch, will be merged into previous
    - (registered) add front end code to forms                   // feature-123-front-end-code branch, will be merged into previous
    - (registered) add front end logic to use the REST endpoints // feature-123-front-end-rest branch, will be merged into previous
    - (registered) add integration tests for headless browser    // feature-123-integration-tests branch, will be merged into previous

You can technically do the above already, by having a main feature branch and functionality specific feature branches all of which get merged into the main feature branch, before then being merged into the main branch after the feature is complete. It's just that people usually live with the view of: "One merge request == one feature", which i don't really think should be the case.

Doesn't seem like such a big problem to make a PR with multiple commits, and then squash and merge into 1 commit after the review.

I see no problem with that if it is a small change that can de deployed safely. (for instance hidden behind a feature-toggle where applicable).

We tend to strive for small, short lived branches. We usually don't want PRs with more than a couple of days work - to keep them small, easy to test. It also makes the amount of changes going to production at any time small.

The best would be if any specific feature is small enough to only be a few days work. But out experience is that it is not allways easy to do that - and sometimes some features will take weeks before they are done.