Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

PR process killing morale and productivity

blackentropy.com

Post reply on HN
Print view View original

191–200 of 224 posts

Re: PR process killing morale and productivity

#191

Post by Aeolun »

donatj wrote: post #23

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've…

I like how when you ask people why they approved something obviously broken, they’ll say something like “Oh, I didn’t really look at it.”

I mean, I do that too once in a while, but if bugs go through you own that.

Re: PR process killing morale and productivity

#192

Post by Aeolun »

donatj wrote: post #40
simmschi wrote: post #28

Earlier quoted context omitted.

Have you tried the code owners feature (assuming you're on Github). IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-) If you don't have specific ownership in your code base I'd start there.

This is all within a relatively small single team. As I said however, I could require review from specific people I know review but they're already at their wits end. Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.

Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?

Re: PR process killing morale and productivity

#193

Post by Aeolun »

DontchaKnowit wrote: post #179
lijok wrote: post #63

Earlier quoted context omitted.

Speaks volumes to the maturity of our work that a 3000 LOC PR is seen not only as acceptable, but expected. Raising a planning change equivalent to a 3000 LOC PR in a civil engineering firm would get your assignment swiftly handed over to someone more competent.

How are you supposed to write new code without hitting a 3k loc pr?

Merge before you’ve written 3000 lines of code?

Re: PR process killing morale and productivity

#194

Post by Aeolun »

thiht wrote: post #171
bsnnkv wrote: post #47

I can't imagine going back to working on codebases in languages that don't come with strongly opinionated defaults. I'm very lucky to spend the majority of my dayjob hours working on Rust code, so everyone just runs cargo fmt && cargo clippy, and this in enforced in CI. You can't even publish a PR until those basic bars have been met. I can't imagine the absolute insanity of working on JS projects where there are mor…

JS has excellent tooling for standardizing code style, Prettier for opinionated formatting and ESLint for everything else. If some teams decide to not use these tools, that’s their mistake.

Or Biome for both. And in 1 second instead of 10 minutes.

Sure, eslint has a few extra rules, but in my opinion it just doesn’t beat getting results at sub second speeds.

Re: PR process killing morale and productivity

#195

Post by Aeolun »

zie1ony wrote: post #170
pavel_lishin wrote: post #139

Earlier quoted context omitted.

Yeah, I've heard arguments for this, and it always feels weird to me. Why go so long without review? That's just giving developers - junior and senior - opportunity to dig themselves into a hole that they don't notice until someone points out, "hey, your last five commits are buggy, but that matters less than the fact that they're implementing the wrong thing."

Good point, but my team is small and has only top engineers with both soft and hard skills. Many times you can do much more with team of 3 than team of 10 ppl.

If the three sit next to one another I agree that PR’s are overkill. If they’re remote it becomes harder I think.

Re: PR process killing morale and productivity

#196

Post by mckn1ght »

BurningFrog wrote: post #53
gboss wrote: post #12

Whenever I encounter a pull request that I find many issues with, I ask to meet with the engineer and review it one on one. More than half of the workplace problems engineers have is due to their introverted nature and their refusal to get on a teams/zoom call to explain their issues and get resolution. Comments are a really poor mechanism for teaching programming best practices.

At my last job, the rule was if the PR is big enough, get on a call and go over it in person. ideally with more than one "reviewer", to break stalemates. Worked really well! I used to be that introverted guy, but a few years of pair programming completely cured that, and I'm now very comfortable discussing design in detail and at length, in a (if I may say so myself) friendly and constructive way. If you've never dis…

I would love this approach, but the people reviewing my PRs are 10 tzs ahead of me, so we basically can’t meet synchronously unless someone is willing to meet early in the morning or late in the evening.

Maybe prerecorded code walkthroughs would help here, but nothing replaces instant face to face pairing. That’s where you can provide somewhat nitpicky feedback but with the empathy that comes with our instinctual responses to body language and voice tone… which over time builds a much better culture IMO.

Re: PR process killing morale and productivity

#197

Post by mckn1ght »

bryanlarsen wrote: post #55
Kinrany wrote: post #33

Earlier quoted context omitted.

It shouldn't be a hard rule, but in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. All a linter does automatically is require adding the "ignore this line" comments that are then visible to humans in diffs.

A linter directive also tells humans that somebody has decided that the function being long is acceptable in this specific location. It's been acknowledged. In many cases, that's all I want.

Then you go from pre-linter arguments about line/function/class length to arguments over whether it’s appropriate to override the linter rule in those same instances.

Re: PR process killing morale and productivity

#198

Post by chromanoid »

Non blocking code reviews ftw

https://itnext.io/optimizing-the-software-development-proces... Optimizing the Software development process for continuous integration and flow of work

https://medium.com/itnext/how-feature-branches-and-pull-requ... How feature branches and pull requests work against best practice

Re: PR process killing morale and productivity

#199

Post by mckn1ght »

kleinsch wrote: post #84
herpdyderp wrote: post #43

Earlier quoted context omitted.

Can confirm GitHub's process is poor for small PRs (if that's what you mean by "multi-commit features"). My team has mostly gotten around this with custom CLI tools (for pushing small PRs chained together) and web apps (for concisely viewing your code review status, both giving and receiving).

I’m a huge fan of https://github.com/VirtusLab/git-machete

Looks great, like a combination of the gh cli and the many options available for maintaining chains of stacked branches!

Re: PR process killing morale and productivity

#200

Post by mckn1ght »

hermanradtke wrote: post #57
praptak wrote: post #56

Combine this with a large timezone difference and you get a (literally) slow motion disaster. Imagine waking up and finding your change thoroughly reviewed but not approved because of a triviality. Not a great day.

My bias is towards merging. Trivial issues can be fixed in a future commit or PR.

This is my life right now and it is terrible, can confirm. I’ve tried advocating for what you say to no avail. Or like, just opening a new PR into my branch, or directly committing to my branch. I’m not territorial, I just want progress. Instead people love to bicker and cover their own ass.
Post reply on HN