Link to publication: https://research.google/pubs/pub49023/
> modern code review provides many benefits ..., including finding defects , knowledge sharing , and improving software maintenance
Absolutely agree, this 3 items are equal in importance. Even most people think that code review is all about "finding defects".
> During code review, developers critically examine each others’ code to improve its quality, share knowledge, and ensure conformance to coding standards.
please focus on bold word - critically. Reviewer should not praise any code he get in review. There should be detailed review with question in mind "Why we need this ?", ...
> Of the common associated factors, four that could be considered causes of pushback in code review were comments on code style, the tone in which feedback was delivered, low familiarity with the code base, and lack of rationale for suggestions or requests made
Yes, agree for all points. BUT people need understand that written communication is considered a bit more rude even writer does not mean anything bad or rude. Feedback should be considered as strive to improve for common benefit/quality (not for benefit of code author!!!). Common code base should serve for all people and NOT for certain person. If reviewer is not familiar with code, author should help to explain all/most or find another reviewer.
> ” Another respondent explained, “there are a lot of reviewers that are unreasonable and take their criticism far beyond what’s documented in the style guide.”
Better to explain in team that style guide can not cover all, even Google Style guide covers most basic items. Author of code should consider seriously feedback from reviewers as it shows exactly a way how this code will be read by other people in future. MAIN POINT: we should no have code that is good only for one person, this will force others to not participate in this project.
One more time: if you allow somebody to merge code as is because your are afraid to "push back" on author because you think he might not contribute to project in future. You do a way more damage to maintainability, because you allow one person to poison participation for others and this person will leave a project for sure because all people eventually stop contributing as their focus and interest is changing.
> “treating code review like a discussion on a web forum, i.e., ignoring the other person as an individual and arguing overly aggressively.” This theme aligns with findings from the interviews, during which participants framed aggressive comments as an initial factor often contributing to feelings of pushback
again, push back is happening to gain more quality of code or more knowledge . Do not afraid push back, it is happening for good. Everyone will benefit from push back, even pushing back is not very pleasant.
> “it was annoying that he was not familiar with the codebase and was questioning stuff about business logic and naming.”
Reviewer is right and author need a bit more patience with reviewer. As code is review is for "knowledge sharing" . If author invest a bit more time in reviewer to explain code base or logic later on this reviewer will catch "defect" in code.
>Several respondents expressed frustration when reviewers requested non-trivial changes without providing a justification. For example, one respondent described how, “some of the comments seemed in general hard to understand, and required a chain of replies from the reviewer/author until the reason why the change is asked is explained,” concluding, “this prevents the person asking for review from learning.”
Most of time justification is skipped when reviewer think it is very obvious. Do not allow frustration to grow in you, ask reviewer to explain. We all people, there might simply distraction happened on reviewer side and he did not finish comment. Call reviewer to get more details, verbal communication is always a way more friendly and detailed.
Some people might be frustrated, but code base is benefiting! Good code will attract more contributors again and again. Even if you lose 20% of contributors due to over high demands in Pull Requests, it is good, as 80% will stay with you.
> Situation 3: An author requests review for a change; the reviewer takes a long time giving rich, detailed feedback; the author then spends substantial time addressing all reviewer comments. All of this is done in just a few rounds of review.
However, situation 3 doesn’t create any of these negative feelings of pushback. It seems developers don’t mind spending an extended time shepherding a change through the process when they receive feedback (even if it came from a very time-consuming review) as long as it is in a succinct number of rounds of feedback. This may be due to reviewers being especially clear about what they want in their initial feedback.
This point works differently in open source and paid jobs. In open source time of reviewer is usually is more limited than time of contributor.
Review process should designed to prioritize review and not allow long waiting and long to-and-from in review. There numerous ways to avoid this (verbal calls, not big PRs, clear testing, PR-preview, ....).
>For instance, the bot could suggest that discussions could be taken offline, that another expert be brought in to provide an outside perspective, or that a change should be broken into smaller parts
yes, yes, yes.
> rejection is important because it helps maintain code quality and that success in contributing to open source depends on being able to handle rejection
yes, it is unfortunate but it is required.
> prior researchers have studied newcomers to open source projects about their initial experiences, finding that newcomers sometimes feel their initial contributions are unwelcome.
unfortunately yes, nobody wants to spend time to explain basic stuff, no that much people can coach others or explain stuff. Take it as part of unfair life and keep pushing to pass newcommers period.
No comments:
Post a Comment