[WIP] Code review edits #3425
[WIP] Code review edits #3425
Conversation
✅ Deploy Preview for the-turing-way ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Good start @llewelld ! I've left some comments, a mix of feedback and stylistic changes but think you're on the right track for sure!
| Doing this can help programmers to see and discuss issues and alternative approaches to tasks, and to learn new tips and tricks. | ||
| This also means code review practices are particularly well-suited to projects with more than one contributor making changes, where each is working on different parts of the code. | ||
| Nonetheless, even the smallest scale projects can harness these approaches with some creative project management. | ||
| Code review is also critical for open source projects, which may have a core set of developers while also inviting contributions from others across the Internet with only an ephemeral relationship with the project. |
There was a problem hiding this comment.
Interested to hear what others think of this sentence - I would switch out ephemeral for something plainer to support translation and I'm also wondering about whether there's a different way to describe newer or more sporadic contributors to an open source project, anyone else want to weigh in?
There was a problem hiding this comment.
I struggled with this wording and I agree, it could do with improvement. I'll certainly remove ephemeral and would value suggestions.
There was a problem hiding this comment.
Maybe go by a minimal example?
.. Who may contribute only to a single aspect once
There was a problem hiding this comment.
Maybe we want to say "inviting and enabling" I think those are different (and the difference is important).
Could also phrase it more like "people who want to make a contribution without becoming a core membet of the project"?
book/website/reproducible-research/reviewing/reviewing-motivation.md
Outdated
Show resolved
Hide resolved
book/website/reproducible-research/reviewing/reviewing-motivation.md
Outdated
Show resolved
Hide resolved
book/website/reproducible-research/reviewing/reviewing-motivation.md
Outdated
Show resolved
Hide resolved
book/website/reproducible-research/reviewing/reviewing-recommend.md
Outdated
Show resolved
Hide resolved
book/website/reproducible-research/reviewing/reviewing-recommend.md
Outdated
Show resolved
Hide resolved
JimMadge
added
the
0-book-dash-nov23
Adds in text concerning the difference between code review in open source and closed projects. Adds in text about the importance of building and running the changes as part of the review process.
Adds additional sections to the "When code review goes wrong" subchapter. Fills out the sections with some initial text, although this still needs to be edited and rationalised.
|
Thank you so much for your comments @Arielle-Bennett. I made some other changes before noticing your comments, so the commits I've just pushed don't try to address your comments. But I really appreciate them, they all look spot on, just what I needed. I'll incorporate them next. |
Changes following an initial read-through.
Incorporate Arielle-Bennett's proposed changes Co-authored-by: Arielle-Bennett <74651964+Arielle-Bennett@users.noreply.github.com>
…wing-motivation.md Co-authored-by: Arielle-Bennett <74651964+Arielle-Bennett@users.noreply.github.com>
| Doing this can help programmers to see and discuss issues and alternative approaches to tasks, and to learn new tips and tricks. | ||
| This also means code review practices are particularly well-suited to projects with more than one contributor making changes, where each is working on different parts of the code. | ||
| Nonetheless, even the smallest scale projects can harness these approaches with some creative project management. | ||
| Code review is also critical for open source projects, which may have a core set of developers while also inviting contributions from others across the Internet with only an ephemeral relationship with the project. |
There was a problem hiding this comment.
Maybe go by a minimal example?
.. Who may contribute only to a single aspect once
| @@ -59,3 +81,8 @@ Peer-to-peer review creates two-way exchange of information across a web strung | |||
|
|
|||
| Reviews conducted in the right spirit (see especially {ref}`here<rr-reviewing-recommendation-be-nice>`) also serve an important purpose in bringing team members together and creating group cohesion. | |||
| In particular, good reviews by core team members of the work of newcomers to a project can help make those newcomers feel welcomed and valued, and encourage their continued participation. | |||
|
|
|||
| For some projects it may even make sense to have code review as a step in the onboarding process, having contributors perform a certain number of reviews before they are invited to contribute in other areas. | |||
There was a problem hiding this comment.
"Code reviews as a step in the onboarding process can serve the purpose of aligning the workstyle of the new member and the rest of the team together before the newcomer is invited to contribute in other areas"?
| Open source projects often also have their own dynamics. | ||
| Contributions may be split between a set of core maintainers, and a larger set of more dispersed contributors, some of whom may have very limited interactions with a project. | ||
| In open source projects it is typically the case that anyone can review a pull request, just as anyone can create one. | ||
| Those who have merge privileges, often limited to core maintainers, inevitably have the final say over what gets added to the code, and so also have the largest role when it comes to code review. |
There was a problem hiding this comment.
More specifically they are responsible for the quality assurance aspect of code reviews
| Perhaps most importantly, always assume good faith on both sides of the review interaction, and always be constructive. | ||
| These principles are true for the review process beyond almost any other project aspect, since it necessarily involves criticism, potentially between two complete strangers. | ||
|
|
||
| Although the review process is asymmetrical, the challenges go in both directions: reviewees may feel their ideas are being misunderstood or criticised unjustly; reviewer's may feel their input is being ignored. | ||
| It's important that all parties in the review process avoid being overly pendantic or territorial. |
There was a problem hiding this comment.
i always like to mention that it actually helps to assume (or remember) that the reviewer and coder inveated substantial effort. Thus that comment/line of code is not there to annoy you, but the result of work
| Although the review process is asymmetrical, the challenges go in both directions: reviewees may feel their ideas are being misunderstood or criticised unjustly; reviewer's may feel their input is being ignored. | ||
| It's important that all parties in the review process avoid being overly pendantic or territorial. | ||
|
|
||
| For an open source project hoping to grow a community it's also important to understand that it can be better to accept poor code contributions than to alienate potential collaborators. |
There was a problem hiding this comment.
this is a very interesting aspect. I know of sqlite where they generally accept no contributions directly but rewrite everything. I wonder if there are studies how this affects contribution, collaboration... But maybe sqlite is a very special case because its highly likely that all contributions are done in paid time
There was a problem hiding this comment.
We discussed that we would prefer a different wording in this sentence. Accepting “poor code” and “alienating contributors” are rather extreme. It is good to mention the tradeoff between code quality and “being nice” to grow the community. Suggestion:
"External contributions might not be at at the standards you set for your own code. When you reject a pull request, make sure to explain, in a constructive manner, why you chose to reject it and what could be done to improve it. If you want to grow your community, this could be a good opportunity to mentor a new contributor."
| Situations in which the reviewer has seniority can lead to a form of "development by proxy", where the reviewer is essentially making their own changes to a piece of code or document through the medium of GitHub comments; the author acts simply as a scribe. | ||
| This is disempowering for the author and also an ineffective use of time. | ||
|
|
||
| If a reviewer is forcing a complete restructure or rewrite of a piece of code in this way, preferable approaches are to either reject the PR and discuss the changes outside of a review, or to accept the changes but also with an associated issue created to specify the other changes required. |
There was a problem hiding this comment.
I think the important part of the sentence here is, that requesting major changes usually benefits from an offline discussion or careful explaination why. Wether the pr is closed or not feels like a matter of preference for me.
Although accepting and creating an issue can be a very good idea. It depends on the situation.
the benefit of having a change merged already may also be, that you understand the production implications before refactoring it
There was a problem hiding this comment.
Does "outside of a review" imply not on the platform you are using for review or not in public?
| Situations in which the author has seniority can lead to a situation where all review comments are dismissed out of hand without explanation or even just ignored completely. | ||
| This is disempowering for the reviewer and leads to disenchantment with the process. | ||
|
|
||
| Power imbalances that can lead to these situations should be avoided as far as possible by aligning processes and selecting reviewers in order to avoid this. |
There was a problem hiding this comment.
i would emphazise that the imbalance should not be avoided but a team should strive to become personally developed enough to be able to benefit even from those reviews. The junior/newcomer might have interesting things to say. And a senior has to learn to do this properly. Avoiding this situation altogether should be the last thing to try.
| When rejecting a change it is essential that the reason is explained for the benefit of the author. | ||
|
|
||
| Remember that not everything has to be resolved in a single pull request. | ||
| If it looks like it's going to be hard to come to a decision on something, create a new issue to deal with it, rather than allow it to block the code merge. |
There was a problem hiding this comment.
actually this can happen from the contributors side as well. A change is reviewed and changes are requested and then the pr is stalling
| When performing a review it can be helpful to have an appreciation of who is ultimately responsible for the changes. | ||
| Reviewers who feel overly responsible are liable to spend too much time and be too critical in their reviews. | ||
|
|
||
| The author of the code is ultimately responsible for its quality, not the reviewer. |
There was a problem hiding this comment.
this depends on the project structure. If the project uses reviews as qa measure than the reviewer / the one who merges is responsible for the quality of the merged code
| 3. The approach encourages you to think holistically about a change, rather than pedantically criticising every small and insignificant detail. | ||
| 4. It discourages overly-hasty and impulsive reactions. | ||
|
|
||
|
|
There was a problem hiding this comment.
i really like this section. I think I would at a section about frustration.
people are frustrated either because the change they made was annoying or the review was hard.
but one should remember to be respectful with the effort of others. When opening a pr i should take proper measures to make the reviewers job pleasent and when getting the review, remember that the reviwer spent time and effort and i have the opertunity to benefit from it.
on the other hand the reviwer needs to remember that the code was the result of (probably) hard work and that a rejected review comment may have better reasons than to annoy the reviewer
There was a problem hiding this comment.
This is looking good. Some great additions to the book and a really thoughtful and balanced discussion 🚀
I feel like perhaps the "When PRs go wrong" sections is missing some sort of summary, or maybe the summary isn't strong enough. The examples are detailed, but the first few paragraphs read a bit like "There can be tension is PRs which is a problem" without explaining why there can be tension or what the harm is.
There is quite a bit of Git and GitHub specific language. I'm not sure if there is a good and agnostic way to say "the process by which your changes get incorporated".
Git/GitHub are probably the most popular tools but maybe we should try and use more generic terms if we can?
| When author and reviewers are working together and in collaboration, the exercise of reviewing can be both enjoyable and beneficial. | ||
| When reviews become antagonistic they can cause immense harm. | ||
|
|
||
| ```{figure} ../../figures/first-pull-request.* |
There was a problem hiding this comment.
It looks like there is both an svg and png image. In the preview the svg is used (🤔).
There are quite a few duplicate images in different formats although I'm not sure why.
| Situations in which the reviewer has seniority can lead to a form of "development by proxy", where the reviewer is essentially making their own changes to a piece of code or document through the medium of GitHub comments; the author acts simply as a scribe. | ||
| This is disempowering for the author and also an ineffective use of time. | ||
|
|
||
| If a reviewer is forcing a complete restructure or rewrite of a piece of code in this way, preferable approaches are to either reject the PR and discuss the changes outside of a review, or to accept the changes but also with an associated issue created to specify the other changes required. |
There was a problem hiding this comment.
Does "outside of a review" imply not on the platform you are using for review or not in public?
| Leaving pull requests half reviewed, from the side of the author or reviewer, can feel disrespectful to other parties who have worked on it. | ||
|
|
||
| If there is any uncertainty about the benefits or purpose of a particular change, it is better to make this clear through a comment, emphasising that you're looking to better understand. | ||
|
|
||
| If a change is ultimately not going to be merged, it is more respectful to reject explicitly than to leave the review incomplete. | ||
| When rejecting a change it is essential that the reason is explained for the benefit of the author. |
There was a problem hiding this comment.
| Leaving pull requests half reviewed, from the side of the author or reviewer, can feel disrespectful to other parties who have worked on it. | |
| If there is any uncertainty about the benefits or purpose of a particular change, it is better to make this clear through a comment, emphasising that you're looking to better understand. | |
| If a change is ultimately not going to be merged, it is more respectful to reject explicitly than to leave the review incomplete. | |
| When rejecting a change it is essential that the reason is explained for the benefit of the author. | |
| Leaving pull requests half reviewed, from the side of the author or reviewer, can feel disrespectful to other parties who have worked on it. | |
| If there is any uncertainty about the benefits or purpose of a particular change, it is better to make this clear through a comment, emphasising that you're looking to better understand. | |
| If a change is ultimately not going to be merged, it is more respectful to reject explicitly than to leave the review incomplete. | |
| When rejecting a change it is essential that the reason is explained for the benefit of the author. |
| As a reviewer it may be necessary to accept the code in a form that you're not happy with. | ||
| In this case, create an issue to fix parts of it that may be problematic in a separate issue. | ||
| It can sometimes be quicker to fix an issue yourself rather than work through someone else to make the change. |
There was a problem hiding this comment.
Can we add a bit more context or guidance here?
I'm thinking, "when is a contribution too bad?", what is reasonable to reject or ask to be changed.
Maybe the advice should be for project maintainers to think about what their 'red lines' are a make that clear?
For example, I could see a contribution which knowingly introduces a bug being reasonably left un-merged. A useful new feature which works but which has bad 'code smell' or is inefficient feels more reasonable to merge now and open issues suggesting improvements.
| When performing a review it can be helpful to have an appreciation of who is ultimately responsible for the changes. | ||
| Reviewers who feel overly responsible are liable to spend too much time and be too critical in their reviews. |
There was a problem hiding this comment.
This is an interesting point.
It might also be worth linking back to the idea of a 'core' team of developers. Although in this section you say the author of the code is responsible, it seems likely that it is the maintainers who will catch the flak of any problems and have to organise a fix. The author may not even get notification that there was a problem.
| @@ -48,6 +48,14 @@ Once you believe changes are complete, the reviewer checks that they do indeed a | |||
|
|
|||
| Once post-review changes have been made to the code, make final updates the comments as needed to complete a history of what has been done and the reasoning behind it. | |||
|
|
|||
| ## Build and Run the Code | |||
|
|
|||
| Code review isn't just about passive review. The majority of pull requests will contain elements that will be hard to understand through inspection of the text documents alone. | |||
There was a problem hiding this comment.
Suggestion: "Code review isn’t just about passive review. Pull requests typically contain elements that can be difficult to understand through inspection of the code alone."
There was a problem hiding this comment.
Nice additions, thanks a lot! We added a few comments here and there. Two suggestions that did not fit into the PR comments:
- We would suggest to soften certain imperatives in the main text (not this PR). It does fit in this PR though. E.g. "You must" -> "You should"
- A chapter on setting a goal for the review would be helpful. Goals can bring focus to the review itself and can clarify what is expected from both sides.
| - Try to have something else to do, and spread the load throughout your | ||
| working day. Don't review for more than an hour at a time, after that the success rate drops quite quickly. | ||
| - Don't review more than 400 lines of code (LOC) at a time, less than 200 LOC is better. Don't review more than 500 LOC/hour. | ||
| working day. |
There was a problem hiding this comment.
Alternative suggestion: "Code reviewing can be quite draining. After more than an hour the success rate of some people drops quite quickly. If possible, consider time-boxing your code reviewing effort, and spreading it over your day. Consider taking breaks and changing to other tasks that will help you be more productive."
| For an open source project hoping to grow a community it's also important to understand that it can be better to accept poor code contributions than to alienate potential collaborators. | ||
|
|
||
| All of this can be very challenging in practice. | ||
| We discuss this further in the next section. |
There was a problem hiding this comment.
Suggestion:
We discuss this further in the section Keep It Collaborative.
Motivation:
Sections might shift around, so it's good to provide a link. Not tested, but according to ( https://stackoverflow.com/questions/27981247/github-markdown-same-page-link ) it should work.
|
|
||
| For an open source project hoping to grow a community it's also important to understand that it can be better to accept poor code contributions than to alienate potential collaborators. | ||
|
|
||
| All of this can be very challenging in practice. |
There was a problem hiding this comment.
Sounds daunting. A suggestion to add more nuance could be instead: "The above mentioned ways of communicating may not come natural to everyone, however practicing will help develop these skills."
| Although the review process is asymmetrical, the challenges go in both directions: reviewees may feel their ideas are being misunderstood or criticised unjustly; reviewer's may feel their input is being ignored. | ||
| It's important that all parties in the review process avoid being overly pendantic or territorial. | ||
|
|
||
| For an open source project hoping to grow a community it's also important to understand that it can be better to accept poor code contributions than to alienate potential collaborators. |
There was a problem hiding this comment.
We discussed that we would prefer a different wording in this sentence. Accepting “poor code” and “alienating contributors” are rather extreme. It is good to mention the tradeoff between code quality and “being nice” to grow the community. Suggestion:
"External contributions might not be at at the standards you set for your own code. When you reject a pull request, make sure to explain, in a constructive manner, why you chose to reject it and what could be done to improve it. If you want to grow your community, this could be a good opportunity to mentor a new contributor."
|
Hey @llewelld - we'd love to see you back at Book Dash this summer to finish off this chapter! ❤️ Are you able to join us this time around? Applications are open until 26 April: https://docs.google.com/forms/d/e/1FAIpQLSdd7Zy6YUxPRpTmvd3yrtE9w7JCb9tA20NVQ-PmtGPsaRsqww/viewform 😄 |
Summary
These changes are aimed to address two areas in the code review sections:
Contributes to #3358
List of changes proposed in this PR (pull-request)
What should a reviewer concentrate their feedback on?
Acknowledging contributors