[gocartStatue]

I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent. Finding out that solution is subpar at this stage is costly, and delaying integration makes ineffective teams.

If you want to enforce standards, automate it (there are multiple highly configurable tools for that). If you want good solutions, pair program. If you want to maintain good codebase, review it periodically (codebase, not some changes to it contained in pull request) and refactor. Use TDD / BDD to ensure the app still works.

[V-2]

"I find post-coding, pre-merge code reviews ineffective, for a change. The code was already written. Time/money spent."

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.

[KronisLV]

> This implies the need for smaller pull requests maybe?

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

You don't actually want these changes to end up in the main branch before they are done. Alone, they provide no actual value and could only confuse people. Furthermore, if there are changes that are needed to the data model after it has already been merged, then you'd also need to do those within a later merge request, or one in the middle, which would just pollute the history and serve as spam, instead of being able to add more commits to the other sub request.

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

(personally i think merges make the most sense, but some people prefer rebasing; it's just an example)

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.

[kashug]

> You don't actually want these changes to end up in the main branch before they are done

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.

[tsimionescu]

While I think this would be nice to have in principle, I think the devil is really in the details. For example, the structure you show for these sub-commits is one I generally hate to review - the data models may look OK when I look at them, but then when I see the implementation code, I realize that the data models were inadequate etc.

[barrkel]

FWIW, that series of small separate requests, reviewed separately, is pretty much how code review works in Google. Each change is attached to the same tracking issue which is what tracks completion, rather than any single or group change being committed.

Yes, alone they don't provide actual value, but they are small (easy to review) and can be trivially / automatically rolled back (database migrations aside) and can be deployed (without being used) safely, and even when they are exercised, it may be via a switch, itself toggled with a pull request which itself can be rolled back in isolation.

Whatever you think is a small change, think even smaller. Think creating a class and stubbing out many of the methods with TODOs.

Delaying the merge until all commits required for the feature wouldn't work in a large monorepo - just keeping small commits up to date can be work when there's enough people working around the clock. There's no feasible way to sustain continuously rebasing in a feature branch model in a large monorepo.

[V-2]

"You don't actually want these changes to end up in the main branch before they are done."

True - and it sounds like a textbook scenario for introducing a feature branch... this approach has worked well for me.

The idea of queueing up "sub-requests" is interesting, but it comes at the cost of creating another layer of complexity (in my view).

[gocartStatue]

Feature branches introduce more complexity than real value (bold statement, right)?

I really enjoy Dave Farley's videos on youtube - "Continuous Delivery" channel. A friend recently sent me this piece from Atlassian: https://www.atlassian.com/git/tutorials/comparing-workflows/...

[KronisLV]

Agreed! I guess what i'm trying to say (in an awfully wordy way) is that we could probably improve the UI for pull/merge requests and gain a lot from it.

I mean, GitHub and GitLab already improved many development workflows with their inline comments (as well as other features, like code recommendations) and it feels like sooner or later more improvements are bound to come!

[WorldMaker]

I don't have as much experience with GitLab, but I actually think Github has a lot of tools in their PR system today for doing "early" or "work in progress" PRs incrementally. You can start a PR on an empty branch. You can apply a label "Work in Progress" and start discussion immediately. If you review a chunk of code the PR is pretty good about showing you what you've already reviewed versus what is new in the most recent update. If for some reason you feel a need to start over and force push an entirely new branch to the PR, Github's PR system handles it better than you would expect (and auto-closes old comments, etc).

I've found the tools are better than people expect them to be and the hard part for me is convincing junior developers that it is "okay" to start PRs "early", that they don't have to feel like they've finished a task to get comments/reviews on work in progress. There's a lot of embarrassment/group politics in the way sometimes from using "PR early" workflows.

[boleary-gl]

GitLab employee here - that is true of GitLab too.

But I agree the bigger challenge than any tooling challenge is getting the team convinced that starting a PR/MR early is not only okay but also the way to operate. Creating physiological safety around it as well as making iteration a part of the day to day workflow is a challenge but an important one.

[frank_bin]

I tend to agree. I prefer design drafts (documents) instead of pair programming, though.