With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.
Commenting and approving pull requests
51–60 of 77 posts
Re: Commenting and approving pull requests
#52With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.
There's two purposes to review. One is to enforce ownership, and the other is to provide mentorship.
Even if you don't have ownership, mentoring is still useful.
I get asked for review by teammembers on an area I have expertise in.
Their solution might work but cause problems later. With review, I can knowledge transfer my lived experience so they don't suffer like I did.
The third purpose to review is stylistic nitpicking and formatting as a simulacrum of actual work. This is useless and turns people off from review.
Re: Commenting and approving pull requests
#53With a robust enough test suite and a team that does TDD and mob programming, code reviews are pretty much obsolete and a waste of time. Everyone's already involved in the coding process as a mob and the tests catch any regressions.
100p agree. Most code reviews devolve to nitpicking anyway. I think it's much more valuable for teammates to review the _design_ and _intent_ rather than the explicit artifacts.
Re: Commenting and approving pull requests
#54I woke up one day and the industry is gating all code changes behind PRs w/ approvals. Anyone still able to commit to main without approval from someone else? I’m even seeing companies require multiple approvals on every PR. It’s insanity. I’m skeptical. Software is as buggy as it ever was. I come across teams shipping terrible quality software, where every line change was approved and reviewed. I come across teams t…
Where I work I can and am still allowed to push to mainline directly, as are all other developers on the team.
Some new colleagues started with PRs and since then it has been a slow move towards using PRs more and more (but still not mandatory).
As of now some colleagues don't typically do PRs and push directly (minority), some decide on a case-by-case basis (I am among those, among a small majority in our team) and some have been using PRs for each and every change from day one (also a minority).
The criticisms by the opponents of PRs are as follows:
1. People relying on PRs too much causes them to propose code which is not production quality as of the PR whereas before (i.e. "no PRs") if your code was not correct, you or someone else either noticed shortly after (checking the tests on CI etc.) or it would make it into the release. I don't believe this to be generally true but sometimes thought similar when reading some PRs. The frequency of large bugs being found by code review in the PR has gone down in recent times which leads me to believe that the colleagues have adjusted their development style to come up with good quality solutions in the first attempt already in most cases.
2. Code review is hard to do in many cases. In our team this is typically resolved by going over a PR together for the “hard cases” (e.g. in a videoconference).
I think PRs are mostly worth the effort because any bug that can be avoided before the release saves a lot of downstream effort (e.g. support) and the effect of building a shared knowledge about the code page is valuable (although hard to quantify).
Re: Commenting and approving pull requests
#55Just want to comment here. I hate reviewer leave a bunch of nits and stamp the PR. This is ambiguous, are these nits asks or just you opinions? What if I dont address all of them. Also folks need to take rejection lightly - your reviewer wants you to address something, thats really it.
Hi, I do this. > are these nits asks or just you opinions? If I've approved the PR, then these are changes I'm asking you to do, but not ordering you to do. You are free to say "no" to my request > What if I dont address all of them Then you will have decided that you don't agree with my recommendation and that's OK. I only ever do this with people I trust - I am trusting you to review each of my nitpicks and make an…
On Github, there is a way to leave individual comments in the code and in addition give a review a summary.
In addition to hitting the "approve" button, I typically spell it out explicitly in this summary: "Please check my comments and see if anything makes sense to implement."
Often, I also take this opportunity to point out the "one" most valuable change in my opinion.
If the developer of the code doesn't find any of the comments to be applicable/usefuly, they can always go ahead and merge it right away.
Re: Commenting and approving pull requests
#56Does anyone not do this? It would be miserable working at a place where nits were blockers and required re-review.
Re: Commenting and approving pull requests
#57We do this too. In my team, my rule is: if it’s better than what’s on master, you approve and merge.
There’s no use making the customer wait for your questions, code style suggestions etc to be addressed.
Even if you request changes, you leave all your comments and make explicit which are the blocking ones and which can be addressed in the future.
Re: Commenting and approving pull requests
#58Earlier quoted context omitted.
Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient. When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.
Write a lint rule. It's often quite easy.
What linter are you assuming I’m running? You can’t always write your own rules.
Re: Commenting and approving pull requests
#59Earlier quoted context omitted.
Linters often don’t provide the constraints you desire. While they’re a great (arguably essential) tool, they often are not sufficient. When a project has well-established patterns, part of the job is to just follow the pattern, whether you like it or not, until you find a reason to 1) change the pattern everywhere, or; 2) make an exception to the pattern with intention.
It's quite simple to write a rule, e.g. for the example I shared https://typescript-eslint.io/rules/array-type . But I've never seen any reputable project arguing against T[] for simple types. In my opinion, that says a lot about the reviewer, not the contributor. If a linter rule doesn't exist, you should argue against the idiomatic convention.
I’m not arguing any particular rule… But what linter are you assuming I’m running? You can’t always write your own rules.
Re: Commenting and approving pull requests
#60[deleted]