31–40 of 79 posts
Worst offender is when a dev decides to reformat code and you can't even differentiate at a glance what is real code change and what is reformatting. Fortunately modern tooling formats code automatically so there's less bikeshedding.
I'm writing a book, "Bytes Of Wisdom: From Start To Merged" that focuses around code reviews. I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view. Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write…
Earlier quoted context omitted.
I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell. I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.
It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review in…
I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater feature is also still one discrete reviewable item.
Earlier quoted context omitted.
I'm not sure I agree? If your code isn't trivial to compartmentalize changes then that might be a code smell. I'd agree keeping the unified context is preferable but it's probably easier to do that by having developers rebase their changes into discrete commits that can be reviewed one by one.
It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review in…
In my experience, this kind of thing happens much more frequently with large PRs.
If a PR is large enough, things will slip through the cracks. If there are too many change requests, even if they're sensible and make sense, things will slip through the cracks. If there's the need for multiple checks by the reviewer, even more things will slip through the cracks.
Also, breaking up PRs in frontend vs backend is not the best idea. Build the feature iteratively if possible.
EDIT: Since the grandparent is talking about reviewing by commit: if those are more reviewable, just use those as the way to separate PRs instead, maybe?
Earlier quoted context omitted.
It's not necessarily about the units of code, it's about the bigger picture. Let's give a hypothetical situation where you split up a PR by the backend and frontend components, but you have some extra fields and endpoints that are not used in your final product. They accidentally get left in because you miss them. I believe the chance that it would be caught in review is significantly higher in a unified PR review in…
> And if you need to have the 2 PRs open side by side, then why split it up in the first place? I agree however I think PR's should be split up. Just not into separate PRs. The solution is to actually start reviewing PRs by commit (you can do this in the PR web interface). Everything is split up and can be reviewed separately but you can still see the final diff and while each discrete unit is reviewable, the greater…
Organizing by clean commits is definitely important.
Worst offender is when a dev decides to reformat code and you can't even differentiate at a glance what is real code change and what is reformatting. Fortunately modern tooling formats code automatically so there's less bikeshedding.
I'm writing a book, "Bytes Of Wisdom: From Start To Merged" that focuses around code reviews. I agree that larger PRs can increase the latency of getting feedback and getting quality feedback. I also have another view. Most code reviews struggle not from the reviewing part and shockingly not the authoring part, but the laying groundwork part. The laying the groundwork part is before any hands to the keyboard to write…
Seems to me like it just can't work. Show a programmer a 5 line program, as the adage goes, and get 5 critiques. Show her a 500 line program, and get a thumbs up; "looks good!"
Really curious to know what keeps professionals from resorting to that timeless adage. Besides sheer chutzpah, how do folks muster the gumption to tell their peers what should have been tackled? Or, do I have my hat on backwards?
Never more than 200 lines.
Where I work it takes several hours to get a PR past all the merge gates. If I have 5 small PRs that all build one on top of the other, it can take me the better part of 2 weeks to get them all merged, counting all the context-switching, tweaking based on comments and relaunching of the merge checks, etc. The only way developers are able to make any kind of velocity in their work is to babysit their PRs morning, even…