Another angle is that a PR with over 300 comments is probably way too big.
Many small focused PRs is IMAO much better than big PRs with weeks or months of work in them.
But that requires fast and friendly PR reviews!
PR process killing morale and productivity
11–20 of 224 posts
Re: PR process killing morale and productivity
#12Whenever 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.
Re: PR process killing morale and productivity
#13> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
Honestly for me it tells me they need to hook up a code auto formatter into their workflow
Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
Re: PR process killing morale and productivity
#14> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
[deleted]
Re: PR process killing morale and productivity
#15> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you t…
"will approve after nitpicks" is the most asinine behavior
Re: PR process killing morale and productivity
#16> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you t…
Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.
Re: PR process killing morale and productivity
#17> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
This literally what the article says.
Re: PR process killing morale and productivity
#18> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
Honestly for me it tells me they need to hook up a code auto formatter into their workflow Forget stylistic nitpicking. Enforce a code quality standard with a linter and formatter and be done with it
Oh 100%. Discussing style within a PR unrelated to changing the styling rules seems like a waste of time.
Re: PR process killing morale and productivity
#19I recommend using something like https://github.com/erikthedeveloper/code-review-emoji-guide in code reviews to signify what your expectation is exactly.
Re: PR process killing morale and productivity
#20Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size lint I'll often chunk off helper functions that are virtually useless outside of that specific function, and in doing so, often make it harder to follow what's going on.
This isn't universally true of course, but it's a "when not if" situation if you have a hard lint limit and it's not very high. (e.g. In my opinion if you're going to have a lint for this, set it to something rather obscene, something that would be extremely hard to cross without doing something clearly awful. In my opinion it's always been set at least 4 times lower than it should be.)