top | item 44656256

(no title)

wirrbel | 7 months ago

> I think this is a very lopsided way of looking at pull requests. They are about a lot more than just trust. Reviewing and being reviewed is a great way of learning from colleagues, making common practices gel in a team, and keeping up to date with changes to the codebase. It’s not just a barrier.

I am old enough that my first 2 jobs were subversion-based (repo side, client side git-svn) and both teams didn't care about branching. It might have to do with how awkward subversion branches felt.

Anyway, we would commit (in git lingo pull-rebase & commit) directly and we basically maintained the id of the last reviewed commit and jointly did PR reviews commit-by-commit with the code on the projector in the office.

We had a joint look at code, everyone voiced their feedback ("I don't understand a variable name like `xzc`"), "where is the unit test", "I read in a blog post recently you are supposed to not use classes...". etc. pp. Sometimes fixes would be pushed right in the PR review session so you had the variable renamed 4 commits further.

Anyway, in retrospect it worked surprisingly well at helping the team to develop a joint understanding of values & virtues that the team would like to maintain in their code base. This might of course be nostalgia of a dev looking back into their junior years.

When we finally got pull-requests, we really felt thrown into the future. It was just great. But after a while I started to miss the direct conversations about code with fellow humans.

And honestly I couldn't tell whether PR really improved the quality of the code base in the long run. They lowered the probability of bad code being committed to the code base, but also lowered the probability for a dev to just fix awkward things while they stumbled over them.

discuss

order

mikepurvis|7 months ago

I miss that too. There’s so many little micro-improvements that are possible when you’re reading code— clarifying a variable name, expanding a comment, adding a test case, and it’s unfortunate when those things are lost under the inertia of it being a pain in the butt to create PRs and get them reviewed.

At the same time, I expect your PM is delighted that you’re not wasting time getting distracted with all that yak shaving nonsense and are instead working on the next burndown ticket that has been assigned a t-shirt size and the appropriate number of story points.

jazzejeff1|7 months ago

Same here. At my previous employer, we’d pair regularly, even on PRs. Larger blocks of work would warrant a mini-standup so the team could all review together in real-time (after an async review with comments). It’s an efficient use of time while acknowledging the need for real-time, humane communication.

tetha|7 months ago

> When we finally got pull-requests, we really felt thrown into the future. It was just great. But after a while I started to miss the direct conversations about code with fellow humans.

Why is this disjoint though?

Our newer colleagues are currently bringing in their first bigger contributions and changes to the config management. We're using short-ish lived feature branches of a week or two with pull requests.

This is good, because I can spend some time understanding what they are doing and I can prepare some ideas and examples for improvements, before we have a call to talk about these changes.

I'm also entirely willing to move my bigger changes into branches and a PR and spend some afternoon with the team to talk about good practices, like structuring commits, naming, ansible code structure, ... and see if other people enjoy that as well. Management wants more stability and broader understanding of our code bases, so moving slower and reading and discussing more code seems up that alley.