Repository navigation
Code review improvements #43584
Description
Activity
We are not in agreement. Considering the state of the CI, landing a PR is not easy because it's often red and it takes 48 hours to land it.
If we want to increase the number of landed PRs.The issue I tried to outline is not about the CI or landing a PR but about the reviews. There are important comments! Absolutely.
I just wonder if we could try to land things when they are functional, tested and have a green CI but the code is not to our own "liking". In a follow-up PR we could improve the latter.I just wonder if we could try to land things when they are functional, tested and have a green CI but the code is not to our own "liking". In a follow-up PR we could improve the latter.
That sounds perfectly reasonable to me, I've done that in the past and seen others do that as well. The drawbacks I've noticed are:
- Sometimes the follow-up PRs doesn't get any reviews. That can be quite frustrating, so it's tempting to instead "hijack" the already reviewed PR.
- Having several PRs can complicate the backport process when something doesn't land cleanly, and/or when we have to revert things.
- Sometimes I promise myself I'm going to work on a follow-up PR, but life happens and I forget about it. Adding a comment on the existing PR is way less work than creating a follow-up PR.
All in all, I don't think we need to settle on anything, creating follow-up PRs is already permitted by our rules, and I don't think it would be reasonable to have a rule to forbid to make non-blocking suggestions on
author readyPRs.Reacted by James M Snell, Jacob Smith, Voltrex, Tobias Nießen and Stephen BelangerAlmost all the feedback I get on my PRs is very valuable, and requested changes are similarly almost always improvements. But, it can sometimes be difficult to determine whether the commenter is seeking it done "now" or just "soon".
I certainly hear what Antoine is saying about the easy of adding a comment on the current PR; I wouldn't want a reviewer to be burdened helping me out. That said, if the comment also included a "could be in a follow-up", that would certainly be much appreciated.
The added complexity to backporting though, I dunno how to solve, and I definitely don't want to pile more work on the people who usually do it.
I recognise the problem for at least three types of personas:
- new contributors who may feel pressurised with overwhelming stylistic reviews
- contributors who are good at analytics / code flow / control flow, and weak at styles
- contributors with large PRs where they really need to worry more about regressions and functional correctness and those aspects may get diluted with interleaving stylistic reviews
One approach would be for the reviewer to be flexible enough to
tagthe stylistic changes asoptionalorfollow-up candidatethat the OP is free to ignore and move on.When I find myself wanting to leave a lot of notes, especially style ones, one thing I’ve done is to open a PR against the original author’s branch. This has a few benefits:
- My review is easier to discuss, as it’s isolated in a separate PR. This also makes it more private.
- There’s less work for the author to accept my suggestions.
- The onus is on me to justify my changes, not on the author to justify rejecting them.
- If/when my PR is eventually accepted, it becomes part of the original PR and lands with it, rather than as a follow-up.
Just thought this technique might help others. I also don’t think it’s too forward to ask pushy reviewers to open PRs against branches for their notes.
Reacted by Gireesh Punathil, Matteo Collina, Jacob Smith, Michaël Zasso, Benjamin Gruenbaum, Vitali Kuzmich, Ray and 木杉Reacted by Jacob SmithSomething that I see sometimes and that I would like to be changed are reviews with a lot of suggestions about code style and other nits that happen before the technical validity of the PR is reviewed. It's a lot of noise1 that can be useless after the PR is refactored. I personally unsubscribe from some pull request notifications because of that.
Footnotes
-
Note for reviewers: please use the Start review/Add review comment feature when there's a high probability for you to add more than one comment. Otherwise each comment can send a separate notification! ↩
Reacted by Gireesh Punathil-
I'm a really recent member, but what I find problematic in my case are the following things:
- Reliability of the CI
- Needing further approvals if adding more commits (or force pushing after a rebase) to the PR.
The second is important because if we relax the constraint (at least according to
git node) we might use the following flow for the PR:- The creator sends the PR
- The majority of the PR (excluding small nit and second/third priority improvements) is approved as of the current rules. The 48 hours period starts as of now.
- The creator will send additional commits without having to wait for the approvals.
- Once CI is green (finger crossed), PR is merged.
This IMHO should guarantee one the best quality/speed PR merge ratio possible I think.
Reacted by Benjamin Gruenbaum2. Needing further approvals if adding more commits (or force pushing after a rebase) to the PR.
FWIW this is not a requirement currently, you can already merge a PR that has approvals and has been open for long enough – the CQ won't do it for security reasons, but it's still possible to do it manually.
Oh, so I'm allowed to overcome the warning. You made my day. Thanks! T_T
As a new contributor to Node.js, I find all reviews too much valuable to agree with idea of not leaving suggestions, no matter of importance.
- Most of reviews and suggestions are totes reasonable and justified, it helps a lot to learn and do better next time.
Learning happens in many different ways: sometimes author might be unfamiliar with better approach, sometimes it's about a fresh language feature, sometimes it's about something newly introduced in Node.js conventions. - Even if a review is completely wrong and unimportant, it is still helpful. One can go through git-blame and usually find an answer to "why it was done like that" in review much more frequently than via comments in code.
- Each line from each PR that landed without any raised question implicitly says "this is how it should be done and how it should look like" and works as a reference example to future PRs from other contributors.
- Sometimes I don't feel confident about code style, especially naming sense. Whenever this fear is justified, the last thing I want from reviewers is tolerance towards my ugly code.
Follow-up PR with stylistic changes that could be made in original PR, aside from technical issues mentioned above, doesn't feel right to me. It disconnects original author from such changes, and has a high chances that their original intentions and rationales will be lost. Sometimes authors might even see such PR as rudeness against their work; and I don't know if pinging them about such changes is a nice idea either.
If a follow-up PR is made, planned or just seriously considered, it does mean that suggestions are not really negligible and changes had to be made. At least in someone's opinion.
The only exception for that is when author plans to do a follow-up rework anyways (for example, if the code requires related but semver-major changes), but it's still nice to have early unresolved reviews for that.As for formal rules, the general idea of not taking nits too seriously seems to be already expressed.
Objectionspart of collaborator guide basically states that even explicitly dissent comments are not blocking.Reviewing pull requestspart of pull requests guide clearly asks to not overwhelm new contributors and recommends marking negligible suggestions withNit:prefix.In fact, the only small contradiction with current guidelines that I see is that there is a recommendation to keep one commit per logical change. Having not only a separate commit but a separate PR for fixups sounds like a thing that should be avoided if possible.
Thus said, I'd like to simply have the statements from these guides to be treated more literally in practice. Authors should feel more easily about nits, leaving them unresolved when needed (e.g. if PR has issues with higher priority, or code requires larger-scale refactoring anyways, etc. etc.). Reviewers should feel easily when their non-blocking feedback is actually non-blocking. Readers should feel easily while reading comments that they consider unimportant.
Unresolved comments are still valuable, what what I think should be outlined is that it's okay to leave them, it's okay to ignore them, and it should meet more acceptance.
Maybe we need a way for explicit and unambiguous marking of suggestions that are expected to be ignored ([FFTI]or something), but this area is too individual so I don't think it would be a great idea.And regarding the overall process, I agree that CI reliability causes much more stalling, frustration and noise than any review process. If this "technical issue" will be somehow closed and therefore PRs could be processed in higher pace, the context of "stylistic issue" might be indirectly changed as well. :)
Reacted by Luigi Pinca, Antoine du Hamel, Feng Yu, Voltrex, Anna Henningsen and Benjamin Gruenbaum- Most of reviews and suggestions are totes reasonable and justified, it helps a lot to learn and do better next time.
My only concern is about stylistic change suggestions - IMO such changes should be enforced by linter rules as much as possible. If it can't be enforced by a linter, I don't think the comment holds much value because it is not enough to enforce the style across the entire codebase consistently. I personally don't find stylistic change suggestions overwhelming, I just feel that enforcing those using a linter is a better approach.
I'm okay with all the other sorts of review comments.
Reacted by Tierney Cyren, Kohei and Juan JoséI agree that CI reliability causes much more stalling, frustration and noise than any review process.
I’ve found that “resume build” gets CI to eventually pass almost 100% of the time. Once or twice it hasn’t worked for me, when say a bug only happens in Windows and so no amount of retrying will get the Windows environments to pass; but the vast majority of the time “resume build” does the trick. Even besides continuing to fight the good fight of making our flakiest tests less flaky, if we could somehow automate triggering “resume build” up to three or five times on failed CI pipelines, I think that alone would eliminate most of the pain most of us feel regarding CI.
Reacted by Jacob SmithAdding in one note having read about half the thread:
One thing I've found helpful is consenting to other maintainers updating my PRs. Often with style changes, I don't care and other people who do care or know what's correct or have a strong preference have historically been willing to just commit the changes they want.
If we do implement something along what's described here, outlining a path for that kind of collaboration might be additionally beneficial.
Reacted by Geoffrey Booth and Jacob SmithI think it would be useful to have a clearer/easier process for landing stuff (especially for new contributors) during times of flaky CI. I've been trying to get people to contribute and they've been very frustrated with the CI and it taking weeks to get a feedback and a green CI.
Reacted by Moshe Atlow and Anna HenningsenW.r.t. CI flakiness: that's hopefully much improved after #43596. Literally half the flakes were on smartos.
Reacted by Benjamin Gruenbaum and Jacob SmithI am wondering could we mark the problem tests as flaky more quickly?
Mark it as flaky doesn't mean we skip them, we could remove the flaky status after we fixed it. I don't think it will harm our project because a flaky test is not a plus, but a minus. It will slow down us and cause the broken window effect.
Now the flaky test problem even stops the pr which fixes the flaky test to merge, such as this one: #43533. We have to rerun the ci again and again.
I talked to @theanarkh offline: he just contribute to node recently and has no privilege to rerun ci. So he has to ask collaborator/triager to rerun ci, again and again, this makes him very frustrated. I will very happy if @theanarkh could share what he thinks in person.
Reacted by RayI agree that CI reliability causes much more stalling, frustration and noise than any review process.
I’ve found that “resume build” gets CI to eventually pass almost 100% of the time. Once or twice it hasn’t worked for me, when say a bug only happens in Windows and so no amount of retrying will get the Windows environments to pass; but the vast majority of the time “resume build” does the trick. Even besides continuing to fight the good fight of making our flakiest tests less flaky, if we could somehow automate triggering “resume build” up to three or five times on failed CI pipelines, I think that alone would eliminate most of the pain most of us feel regarding CI.
What I've done in the past is literally tag a test flaky; on a CI failure, check for the presence of the tag (when present, re-try N-times).
One thing I've found helpful is consenting to other maintainers updating my PRs. Often with style changes, I don't care and other people who do care or know what's correct or have a strong preference have historically been willing to just commit the changes they want.
I think this would depend on who: if a random collaborator did that on my PR, I'd be a little offended, but if it was someone from my team, that'd be fine.
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
What is the problem this feature will solve?
Our code reviews are frequently taking a big burden for the authors where small issues are requested to be changed that are mostly stylistic or not really important for the PR to land. I believe it's often not easy to step back and not to ask for something to be changed when it's not necessary but it feels "right". I personally am definitely also guilty of this.
@nodejs/tsc @nodejs/collaborators please weight in as this mainly is about all of us. Please also share if you feel similar / disagree with my view.
What is the feature you are proposing to solve the problem?
In many cases we could likely have a follow-up PR on our own and get things done faster and have a nicer developer experience.
Asking for things that are not required just often does not feel right to me as it makes it difficult to contribute to the project.
There are likely more ways to do this, I just wanted to quickly write this down and get early opinions.