Rework code review section #226
Conversation
As per carpentries-incubator#217 adjusted the structure of the document according to the sketch. Removed sections that won't be needed, and added place holder sections for the new components.
Includes solution of the kinds of problem to spot
Swapped the review exercise round so putting comments on other peoples repositories as this doesn't require setting up collaborators, making things simpler.
Remove the bullet points from the end section that are now covered by this section.
Removed associated steps from the list at the end
|
I've added the last of the missing sections. This is ready for a structural / content review. Once I've got that I will do a line by line edit to make sure it makes sense / flows. |
thomaskileyukaea
left a comment
thomaskileyukaea
left a comment
There was a problem hiding this comment.
Explanation of why I removed certain sections / exercises as well as some ❓ questions for the maintainers that we should think about before merging this.
| > ## Group Exercise: Advantages of Code Review | ||
| > Discuss as a group: what do you think are the reasons behind, and advantages of, code review? | ||
| > > ## Solution | ||
| > > The purposes of code review include: | ||
| > > - improving internal code readability, understandability, quality and maintainability | ||
| > > - checking for coding standards compliance, code uniformity and consistency | ||
| > > - checking for test coverage and detecting bugs and code defects early | ||
| > > - detecting performance problems and identifying code optimisation points | ||
| > > - finding alternative/better solutions. | ||
| > > | ||
| > > An effective code review prevents errors from creeping into your software | ||
| > > by improving code quality at an early stage of the software development process. | ||
| > > It helps with learning, i.e. sharing knowledge about the codebase, | ||
| > > solution approaches, | ||
| > > expectations regarding quality, | ||
| > > coding standards, etc. | ||
| > > Developers use code review feedback from more senior developers | ||
| > > to improve their own coding practices and expertise. | ||
| > > Finally, it helps increase the sense of collective code ownership and responsibility, | ||
| > > which in turn helps increase the "bus factor" | ||
| > > and reduce the risk resulting from information and capabilities | ||
| > > being held by a single person "responsible" for a certain part of the codebase | ||
| > > and not being shared among team members. | ||
| > {: .solution} | ||
| {: .challenge} | ||
|
|
There was a problem hiding this comment.
I didn't think this exercise added much and I wanted to add other more exercises - but I'm happy to discuss.
There was a problem hiding this comment.
I tend to agree that this doesn't add much. It has a large amount of overlap with the similar
"reflective" exercise at the very end of the episode, which I do think adds some value because it
gets participants to actively think about how they might use code review in their own work.
Moreover, this exercise comes quite early in the lesson before much has been said about code review. I imagine
anyone who hasn't come across code review before won't have much to contribute to the discussion at
this point.
However, I haven't run the course with this episode, so it would be good to get some input from
@anenadic @steve-crouch @smangham @douglowe about whether this exercise generated any valuable
discussion about code review.
| There are several **code review techniques** with various degree of formality | ||
| and the use of a technical infrastructure, including: | ||
|
|
||
| - **Over-the-shoulder code review** | ||
| is the most common and informal of code review techniques and involves | ||
| one or more team members standing over the code author's shoulder | ||
| while the author walks the reviewers through a set of code changes. | ||
| - **Email pass-around code review** | ||
| is another form of lightweight code review where the code author | ||
| packages up a set of changes and files and sends them over to reviewers via email. | ||
| Reviewers examine the files and differences against the code base, | ||
| ask questions and discuss with the author and other developers, | ||
| and suggest changes over email. | ||
| The difficult part of this process is the manual collection the files under review | ||
| and noting differences. | ||
| - **Pair programming** | ||
| is a code development process that incorporates continuous code review - | ||
| two developers sit together at a computer, | ||
| but only one of them actively codes whereas the other provides real-time feedback. | ||
| It is a great way to inspect new code and train developers, | ||
| especially if an experienced team member walks a younger developer through the new code, | ||
| providing explanations and suggestions through a conversation. | ||
| It is conducted in-person and synchronously but it can be time-consuming | ||
| as the reviewer cannot do any other work during the pair programming period. | ||
| - **Fagan code inspection** | ||
| is a formal and heavyweight process of finding defects in specifications or designs | ||
| during various phases of the software development process. | ||
| There are several roles taken by different team members in a Fagan inspection | ||
| and each inspection is a formal 7-step process with a predefined entry and exit criteria. | ||
| See [Fagan inspection](https://en.wikipedia.org/wiki/Fagan_inspection) | ||
| for full details on this method. | ||
| - **Tool-assisted code review** | ||
| process uses a specialised tool to facilitate the process of code review, | ||
| which typically helps with the following tasks: | ||
| (1) collecting and displaying the updated files and highlighting what has changed, | ||
| (2) facilitating a conversation between team members (reviewers and developers), and | ||
| (3) allowing code administrators and product managers | ||
| a certain control and overview of the code development workflow. | ||
| Modern tools may provide a handful of other functionalities too, such as metrics | ||
| (e.g. inspection rate, defect rate, defect density). | ||
|
|
||
| Each of the above techniques have their pros and cons and varying degrees practicality - | ||
| it is up to the team to decide which ones are most suitable for the project and when to use them. |
There was a problem hiding this comment.
I don't see how this section contributes to the goals, so removing this gives more time to focus on the kind of review system we're recommending - tool assisted review.
There was a problem hiding this comment.
I agree that the level of detail isn't needed for the objectives of this section. However, it might
be nice to mention somewhere that tool-assisted code review on GitHub isn't the only way to do code
review, and then list these types so that learners can go look at them if they are interested. The
next paragraph seems like the obvious place to do this.
There was a problem hiding this comment.
Ah, I see that this is briefly mentioned in one of the sections below now, so ignore my comment
above. I'm now fine to get rid of this section.
There was a problem hiding this comment.
I will make this adjustment as I think you're right, it is nice to have some other terms to look up, but with less detail.
| To achieve this, the following steps are needed. | ||
|
|
||
| #### Step 1: Adding Collaborators to a Shared Repository | ||
|
|
||
| You need to add the other team member(s) as collaborator(s) on your repository | ||
| to enable them to create branches and pull requests. | ||
| To do so, each repository owner needs to: | ||
|
|
||
| 1. Head over to Settings section of your software project's repository in GitHub. | ||
| {: .image-with-shadow width="900px"} | ||
| 2. Select the **vertical** tab 'Collaborators' from the left and click the 'Add people' button. | ||
| {: .image-with-shadow width="900px"} | ||
| 3. Add your collaborator(s) by their GitHub username(s), full name(s) or email address(es). | ||
| {: .image-with-shadow width="900px"} | ||
| 4. Collaborator(s) will be notified of your invitation to join your repository | ||
| based on their notification preferences. | ||
| 5. Once they accept the invitation, they will have the collaborator-level access to your repository | ||
| and will show up in the list of your collaborators. | ||
|
|
||
| See the full details on | ||
| [collaborator permissions for personal repositories](https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-user-account-settings/permission-levels-for-a-user-account-repository) | ||
| to understand what collaborators will be able to do within your repository. | ||
| Note that repositories owned by an organisation have a | ||
| [more granular access control](https://docs.github.com/en/get-started/learning-about-github/access-permissions-on-github) | ||
| compared to that of personal repositories. |
There was a problem hiding this comment.
I restructured the examples so they don't require adding collaborators. I would be tempted to move this into the extras though, as it might be useful in some instances to know how to do this.
There was a problem hiding this comment.
The language above (L149 in changed file) still mentions adding collaborators and then the
description of the exercise is still claiming that the learner will implement the tests of SR1.1.1
or 1.2.1. I am assuming that this still needs to be reworked? Just wanted to flag in case it slipped
off your radar.
My other concern with removing this particular section about adding collaborators is that future
exercises will require participants to work in groups on a single shared repository. If we don't do
the steps of adding a collaborator here, we will need to more it to a more suitable location. I
think Assessing Software for Suitability and
Improvement
is probably the natural place.
| #### Step 2: Preparing Your Local Environment for a Pull Request | ||
|
|
||
| 1. Obtain the GitHub URL of the shared repository you will be working on and clone it locally | ||
| (make sure you do it outside your software repository's folder you have been working on so far). | ||
| This will create a copy of the repository locally on your machine | ||
| along with all of its (remote) branches. | ||
| ~~~ | ||
| $ git clone <remote-repo-url> | ||
| $ cd <remote-repo-name> | ||
| ~~~ | ||
| {: .language-bash} | ||
| 2. Check with the repository owner (your team member) | ||
| which feature (SR1.1.1 or SR1.2.1) they implemented in the | ||
| [previous exercise](/32-software-design/index.html#implement-requirements) | ||
| and what is the name of the branch they worked on. | ||
| Let's assume the name of the branch was `feature-x` | ||
| (you should amend the branch name for your case accordingly). | ||
| 3. Your task is to add tests for the code on `feature-x` branch. | ||
| You should do so on a separate branch called `feature-x-tests`, | ||
| which will branch off `feature-x`. | ||
| This is to enable you later on to create a pull request | ||
| from your `feature-x-tests` branch with your changes | ||
| that can then easily be reviewed and compared with `feature-x` | ||
| by the team member who created it. | ||
|
|
||
| To do so, branch off a new local branch `feature-x-tests` from the remote `feature-x` branch | ||
| (making sure you use the branch names that match your case). | ||
| Also note that, while we say "remote" branch `feature-x` - | ||
| you have actually obtained it locally on your machine when you cloned the remote repository. | ||
| ~~~ | ||
| $ git checkout -b feature-x-tests origin/feature-x | ||
| ~~~ | ||
| {: .language-bash} | ||
|
|
||
| You are now located in the new (local) `feature-x-tests` branch | ||
| and are ready to start adding your code. | ||
|
|
There was a problem hiding this comment.
Now the exercises are reviewing a fixed changeset this is no longer required.
| > ## Exercise: Implement Tests for the New Feature | ||
| > Look back at the | ||
| > [solution requirements](/31-software-requirements/index.html#solution-requirements) | ||
| > (SR1.1.1 or SR1.2.1) | ||
| > for the feature that was implemented in your shared repository. | ||
| > Implement tests against the appropriate specification in your local feature branch. | ||
| > | ||
| > *Note: Try not to not fall into the trap of | ||
| > writing the tests to test the existing code/implementation - | ||
| > you should write the tests to make sure the code satisfies the requirements | ||
| > regardless of the actual implementation. | ||
| > You can treat the implementation as a | ||
| > [black box](https://en.wikipedia.org/wiki/Black-box_testing) - | ||
| > a typical approach to software testing - | ||
| > as a way to make sure it is properly tested against its requirements | ||
| > without introducing assumptions into the tests about its implementation.* | ||
| {: .challenge} | ||
|
|
||
| > ## Testing Based on Requirements | ||
| > Tests should test functionality, | ||
| > which stem from the software requirements, | ||
| > rather than an implementation. | ||
| > Tests can be seen as a reflection of those requirements - | ||
| > checking if the requirements are satisfied. | ||
| {: .callout} | ||
|
|
||
| Remember to commit your new code to your branch `feature-x-tests`. | ||
|
|
||
| ~~~ | ||
| $ git add -A | ||
| $ git commit -m "Added tests for feature-x." | ||
| ~~~ | ||
| {: .language-bash} |
There was a problem hiding this comment.
As above - reader is no longer required to write some tests for someone else to review
_episodes/41-code-review.md
Outdated
| ### What not to look for | ||
|
|
||
| The overriding priority for reviewing code should be making sure progress is being made - | ||
| don't let perfect be the enemy of good here. Further, research has shown that the first hour | ||
| of reviewing code is the most effective, with diminishing returns after that. | ||
|
|
||
| To that end, here are a few things you shouldn't be trying to spot when reviewing: | ||
|
|
||
| #### Linting issues, or anything else that an automated tool can spot | ||
|
|
||
| Get the CI to do this - this will save the reviewer time, be more accurate | ||
| and avoid needless conflict. | ||
|
|
||
| #### Bugs | ||
|
|
||
| It is easier to make sure there are sufficient tests - you are not an accurate | ||
| computer simulator anyway. | ||
|
|
||
| #### Issues that pre-date the change | ||
|
|
||
| You may spot something that the reviewer didn't introduce and think they could | ||
| fix it while in the area, but this can be a rabbit hole. A better approach would be to | ||
| raise a PR after this one has been merged fixing the thing you spotted. |
There was a problem hiding this comment.
❓ I'm not sure this is actually that useful so I'm tempted to just cut it.
There was a problem hiding this comment.
I sort of like this because it helps focus what code review should be about. That being said, it
isn't as important as the previous tips about what to look for, so perhaps we assess this based on
how long the section takes to deliver?
| ## Designing a review process | ||
|
|
||
| To maximize the benefit of a code review it needs a process that is followed by everyone | ||
| involved, and that everyone believes that process provides value. | ||
|
|
||
| One way to foster this is to design the process as a team. When you're doing this you | ||
| should consider: | ||
|
|
||
| * Do all changes need to go through code review | ||
| * What technologies will you use to manage the review process | ||
| * How quickly do you expect someone to review the code once you've raised a PR? | ||
| * How long should be spent reviewing code? | ||
| * What kind of issues are (and aren't) appropriate to raise in a PR? | ||
| * How will someone know when they are expected to take action (e.g. review a PR). | ||
|
|
||
| Once you've introduced a review process, you should monitor (either formally or | ||
| informally) how well it is working. | ||
|
|
||
| It is important that reviews are processed quickly, to avoid costly context switching. | ||
| We recommend aiming for 3 hours to get a first review, with the PR being merged the same | ||
| day in most cases. If you are regularly missing these targets, then you should review | ||
| where things are getting stuck and work out what you can do to move things along. |
There was a problem hiding this comment.
❓ Here I am assuming the reader is part of a team. However, I remember in discussions we talked how lots of users of this course are working independently. How do those scientists who are working independently use the code review section? Do they find someone else in their dept to review their code, reviewing it themselves, or just skipping this section?
There was a problem hiding this comment.
This is a good question. As someone who has lone coded in various scientific settings, I still use
merge/pull requests as a gate check on what I have just done. Of course, I haven't done that in
every instance, but it is a nice idea to strive for.
In terms of actual code review, this is tough. I was part of a community effort to try and define
what code review in research might look like: https://dev-review.readthedocs.io/en/latest/index.html
There, we did recommend trying to find someone to review your code. That person could be in your
research group (but just not working directly on what you are). We could link to that page here do
deal with the case of solo researchers working on code.
bielsnohr
left a comment
bielsnohr
left a comment
There was a problem hiding this comment.
Overall, I really like where this is heading. The exercise based on a pre-determined changeset should help better achieve the objectives of this section. In the previous version, not having control over the incoming changes meant that learners might not see many of the points being raised in this section. So, this new approach gets my full endorsement.
It will be great to see how the section flows once the wording around this new exercise is added.
| > ## Group Exercise: Advantages of Code Review | ||
| > Discuss as a group: what do you think are the reasons behind, and advantages of, code review? | ||
| > > ## Solution | ||
| > > The purposes of code review include: | ||
| > > - improving internal code readability, understandability, quality and maintainability | ||
| > > - checking for coding standards compliance, code uniformity and consistency | ||
| > > - checking for test coverage and detecting bugs and code defects early | ||
| > > - detecting performance problems and identifying code optimisation points | ||
| > > - finding alternative/better solutions. | ||
| > > | ||
| > > An effective code review prevents errors from creeping into your software | ||
| > > by improving code quality at an early stage of the software development process. | ||
| > > It helps with learning, i.e. sharing knowledge about the codebase, | ||
| > > solution approaches, | ||
| > > expectations regarding quality, | ||
| > > coding standards, etc. | ||
| > > Developers use code review feedback from more senior developers | ||
| > > to improve their own coding practices and expertise. | ||
| > > Finally, it helps increase the sense of collective code ownership and responsibility, | ||
| > > which in turn helps increase the "bus factor" | ||
| > > and reduce the risk resulting from information and capabilities | ||
| > > being held by a single person "responsible" for a certain part of the codebase | ||
| > > and not being shared among team members. | ||
| > {: .solution} | ||
| {: .challenge} | ||
|
|
There was a problem hiding this comment.
I tend to agree that this doesn't add much. It has a large amount of overlap with the similar
"reflective" exercise at the very end of the episode, which I do think adds some value because it
gets participants to actively think about how they might use code review in their own work.
Moreover, this exercise comes quite early in the lesson before much has been said about code review. I imagine
anyone who hasn't come across code review before won't have much to contribute to the discussion at
this point.
However, I haven't run the course with this episode, so it would be good to get some input from
@anenadic @steve-crouch @smangham @douglowe about whether this exercise generated any valuable
discussion about code review.
| There are several **code review techniques** with various degree of formality | ||
| and the use of a technical infrastructure, including: | ||
|
|
||
| - **Over-the-shoulder code review** | ||
| is the most common and informal of code review techniques and involves | ||
| one or more team members standing over the code author's shoulder | ||
| while the author walks the reviewers through a set of code changes. | ||
| - **Email pass-around code review** | ||
| is another form of lightweight code review where the code author | ||
| packages up a set of changes and files and sends them over to reviewers via email. | ||
| Reviewers examine the files and differences against the code base, | ||
| ask questions and discuss with the author and other developers, | ||
| and suggest changes over email. | ||
| The difficult part of this process is the manual collection the files under review | ||
| and noting differences. | ||
| - **Pair programming** | ||
| is a code development process that incorporates continuous code review - | ||
| two developers sit together at a computer, | ||
| but only one of them actively codes whereas the other provides real-time feedback. | ||
| It is a great way to inspect new code and train developers, | ||
| especially if an experienced team member walks a younger developer through the new code, | ||
| providing explanations and suggestions through a conversation. | ||
| It is conducted in-person and synchronously but it can be time-consuming | ||
| as the reviewer cannot do any other work during the pair programming period. | ||
| - **Fagan code inspection** | ||
| is a formal and heavyweight process of finding defects in specifications or designs | ||
| during various phases of the software development process. | ||
| There are several roles taken by different team members in a Fagan inspection | ||
| and each inspection is a formal 7-step process with a predefined entry and exit criteria. | ||
| See [Fagan inspection](https://en.wikipedia.org/wiki/Fagan_inspection) | ||
| for full details on this method. | ||
| - **Tool-assisted code review** | ||
| process uses a specialised tool to facilitate the process of code review, | ||
| which typically helps with the following tasks: | ||
| (1) collecting and displaying the updated files and highlighting what has changed, | ||
| (2) facilitating a conversation between team members (reviewers and developers), and | ||
| (3) allowing code administrators and product managers | ||
| a certain control and overview of the code development workflow. | ||
| Modern tools may provide a handful of other functionalities too, such as metrics | ||
| (e.g. inspection rate, defect rate, defect density). | ||
|
|
||
| Each of the above techniques have their pros and cons and varying degrees practicality - | ||
| it is up to the team to decide which ones are most suitable for the project and when to use them. |
There was a problem hiding this comment.
I agree that the level of detail isn't needed for the objectives of this section. However, it might
be nice to mention somewhere that tool-assisted code review on GitHub isn't the only way to do code
review, and then list these types so that learners can go look at them if they are interested. The
next paragraph seems like the obvious place to do this.
| To achieve this, the following steps are needed. | ||
|
|
||
| #### Step 1: Adding Collaborators to a Shared Repository | ||
|
|
||
| You need to add the other team member(s) as collaborator(s) on your repository | ||
| to enable them to create branches and pull requests. | ||
| To do so, each repository owner needs to: | ||
|
|
||
| 1. Head over to Settings section of your software project's repository in GitHub. | ||
| {: .image-with-shadow width="900px"} | ||
| 2. Select the **vertical** tab 'Collaborators' from the left and click the 'Add people' button. | ||
| {: .image-with-shadow width="900px"} | ||
| 3. Add your collaborator(s) by their GitHub username(s), full name(s) or email address(es). | ||
| {: .image-with-shadow width="900px"} | ||
| 4. Collaborator(s) will be notified of your invitation to join your repository | ||
| based on their notification preferences. | ||
| 5. Once they accept the invitation, they will have the collaborator-level access to your repository | ||
| and will show up in the list of your collaborators. | ||
|
|
||
| See the full details on | ||
| [collaborator permissions for personal repositories](https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-user-account-settings/permission-levels-for-a-user-account-repository) | ||
| to understand what collaborators will be able to do within your repository. | ||
| Note that repositories owned by an organisation have a | ||
| [more granular access control](https://docs.github.com/en/get-started/learning-about-github/access-permissions-on-github) | ||
| compared to that of personal repositories. |
There was a problem hiding this comment.
The language above (L149 in changed file) still mentions adding collaborators and then the
description of the exercise is still claiming that the learner will implement the tests of SR1.1.1
or 1.2.1. I am assuming that this still needs to be reworked? Just wanted to flag in case it slipped
off your radar.
My other concern with removing this particular section about adding collaborators is that future
exercises will require participants to work in groups on a single shared repository. If we don't do
the steps of adding a collaborator here, we will need to more it to a more suitable location. I
think Assessing Software for Suitability and
Improvement
is probably the natural place.
_episodes/41-code-review.md
Outdated
| When you have finished adding your tests | ||
| and committed the changes to your local `feature-x-tests`, |
There was a problem hiding this comment.
Will the content below will also need to be removed or updated for the new exercise?
There was a problem hiding this comment.
Yes - this is now done.
_episodes/41-code-review.md
Outdated
| ### What not to look for | ||
|
|
||
| The overriding priority for reviewing code should be making sure progress is being made - | ||
| don't let perfect be the enemy of good here. Further, research has shown that the first hour | ||
| of reviewing code is the most effective, with diminishing returns after that. | ||
|
|
||
| To that end, here are a few things you shouldn't be trying to spot when reviewing: | ||
|
|
||
| #### Linting issues, or anything else that an automated tool can spot | ||
|
|
||
| Get the CI to do this - this will save the reviewer time, be more accurate | ||
| and avoid needless conflict. | ||
|
|
||
| #### Bugs | ||
|
|
||
| It is easier to make sure there are sufficient tests - you are not an accurate | ||
| computer simulator anyway. | ||
|
|
||
| #### Issues that pre-date the change | ||
|
|
||
| You may spot something that the reviewer didn't introduce and think they could | ||
| fix it while in the area, but this can be a rabbit hole. A better approach would be to | ||
| raise a PR after this one has been merged fixing the thing you spotted. |
There was a problem hiding this comment.
I sort of like this because it helps focus what code review should be about. That being said, it
isn't as important as the previous tips about what to look for, so perhaps we assess this based on
how long the section takes to deliver?
_episodes/41-code-review.md
Outdated
| > have all been addressed, you can approve the PR. | ||
| {: .challenge} | ||
|
|
||
| #### Step 6: Closing a Pull Request |
There was a problem hiding this comment.
Will need to get rid of "Step 6" and make sure the heading level is in line with the above.
_episodes/41-code-review.md
Outdated
| The most important thing to keep in mind is how long your pull request is. Smaller | ||
| changes, that just make one small improvement, will be much quicker and easier to | ||
| review. There is no golden rule, but 100 - 500 lines is a good size. More than | ||
| 1000 lines is almost certainly too big. | ||
|
|
||
| Even within a single review, try to keep each commit to be making one logical change. | ||
| This can help if your review would otherwise be too large. In particular, if you've | ||
| reformatted, refactored and changed the behavior of the code make sure each of these | ||
| is in a separate commit (i.e reformat the code, commit, refactor the code, commit, alter | ||
| the behavior of the code, commit). | ||
|
|
||
| Make sure you write a clear description of the content and purpose of the change. | ||
| This should be provided as the pull request description. | ||
| This should provide the context that reading the code will make more sense. | ||
|
|
||
| It is also a good idea to review your code yourself. In doing this you will spot | ||
| the more obvious issues with your code, allowing your reviewer to focus on the | ||
| things you cannot spot. |
There was a problem hiding this comment.
The paragraphs need some semantic breaks at periods.
_episodes/41-code-review.md
Outdated
| Code is written by humans (mostly!), and code review is a form of communication. As such | ||
| empathy is really important for effective reviewing. | ||
|
|
||
| When reviewing code, it can be sometimes frustrating when code is confusing, particularly | ||
| as it will be implemented differently to how you would have done it. However, it is important | ||
| as a reviewer to be compassionate to the person whose code you are reviewing. Specifically: |
| ## Designing a review process | ||
|
|
||
| To maximize the benefit of a code review it needs a process that is followed by everyone | ||
| involved, and that everyone believes that process provides value. | ||
|
|
||
| One way to foster this is to design the process as a team. When you're doing this you | ||
| should consider: | ||
|
|
||
| * Do all changes need to go through code review | ||
| * What technologies will you use to manage the review process | ||
| * How quickly do you expect someone to review the code once you've raised a PR? | ||
| * How long should be spent reviewing code? | ||
| * What kind of issues are (and aren't) appropriate to raise in a PR? | ||
| * How will someone know when they are expected to take action (e.g. review a PR). | ||
|
|
||
| Once you've introduced a review process, you should monitor (either formally or | ||
| informally) how well it is working. | ||
|
|
||
| It is important that reviews are processed quickly, to avoid costly context switching. | ||
| We recommend aiming for 3 hours to get a first review, with the PR being merged the same | ||
| day in most cases. If you are regularly missing these targets, then you should review | ||
| where things are getting stuck and work out what you can do to move things along. |
There was a problem hiding this comment.
This is a good question. As someone who has lone coded in various scientific settings, I still use
merge/pull requests as a gate check on what I have just done. Of course, I haven't done that in
every instance, but it is a nice idea to strive for.
In terms of actual code review, this is tough. I was part of a community effort to try and define
what code review in research might look like: https://dev-review.readthedocs.io/en/latest/index.html
There, we did recommend trying to find someone to review your code. That person could be in your
research group (but just not working directly on what you are). We could link to that page here do
deal with the case of solo researchers working on code.
Without going into detail, shows that there are other options that can be looked into.
Exercise has changed, so update the text to reflect this. It is a little confusing that you raise a PR on your own repository, and then comment on a different one, but this does allow for not needing to work out the collaboration.
Rather than have an unsorted list, work the useful recommendations into relevant sections of the course.
This is covered in Designing a review process
These are important points that are covered by other parts of the episode, so include the relevant source material.
This is again a bit of an aside so a callout makes it clearer we are taking a diversion. Also compressed the content quite substantially - this would make good content for a discussion but is a bit verbose for the lesson format.
Clearer than mulitple single sentence paragraphs
This works as a good overview of the section so it is easy to see what the points are, before diving into more detail
This way if the timeline gets longer it will stay a consistent font size as the diagram gets bigger
This text can be used in mermaid.live to regenerate the diagram
|
Thanks @anenadic for the review - it was really helpful. I've tried to address your comments - or else have replied with further clarifications. Do let me know if you have any further feedback. I've merged the latest gh-pages into this so that it has the same collaborative code models. |
|
I also looked into using Mermaid diagrams directly - at first it seemed tricky, but ended up being quite straight forward - here's a PR on my fork showing how it could be done: thomaskileyukaea#1 If we did go down this route, we could convert a few of the git diagrams into mermaid diagrams too, which would make maintenance easier. |
|
Comments from maintainer sprint:
|
Aha! Great that you were able to get this working. Of course, I should have remembered the solution of just injecting the required Javascript somewhere in the HTML. One gets quite used to everything being done with packages/gems/etc... I would vote that we do go down the route of adding Mermaid. The diagram for this lesson looks great, see the bottom of this comment. GitHub already supports Mermaid, so the below is actually a rendering of the source code. Also, as @thomaskileyukaea mentions, we can convert other diagrams to Mermaid and it will make maintenance of the website easier (no more fiddling with svg, regenerating a png/jpeg, making sure they are in the right folder, etc). Could I request that we get a maintainer vote on this? @anenadic I'll have a look through the governance document we are working on, but email list probably best? %% Must disable useMaxWidth to have the diagram centre
%%{init: { 'sequence': {'useMaxWidth':false} } }%%
sequenceDiagram
participant A as Author
participant R as Reviewer
A->>A: Write some code
A->>R: Raise a pull request
R->>R: Add comments to a review
R->>A: Submit a review
loop Until approved
A->>R: Address or respond to review comments
R->>A: Clarify or resolve comments
end
R->>A: Approve pull request
A->>A: Merge pull request
|
@anenadic I think because of various AL schedules, it has been difficult to arrange a synchronous call about the bigger changes you mention above. By "bigger changes" are you referring to the fact that steps in some of the initial episodes for obtaining the project would need to be modified? If that is the extent of the changes, then I think it is quite manageable. There is the issue of regenerating some images of the GitHub interface, but I can assist with that. As you suggest, it is probably best that we accept this MR in its current state into the |
bielsnohr
left a comment
bielsnohr
left a comment
There was a problem hiding this comment.
As mentioned in comments above, I think we are fine to merge this into the dev branch and then take some time to make the edits to earlier parts of the lesson to advise on using forking.
By extending the layout we can load the mermaid script directly to use diagrams in the lesson
By default the diagrams generated by mermaid use max-width property. This means that they cannot be centre aligned in their parent container. Disabling this combined with setting the element to be 100% with auto margins will centre the diagram.
No reason to have a grey background on the diagrams
The wrong base template had been used for episodes, resulting in the schedule appearing at the bottom.
|
✔️ Addressed issue where the schedule appeared at the bottom after the mermaid change. |
|
Summary of outcome from Maintainer Meeting 2023-10-25:
|
We can either re-create a dev branch for this PR or have a new branch of the same name |
I think creating a new branch within our repo to hold this work is the best next step. I'll do that shortly. It would be good if we could collect the concrete items that need to change in this issue: #217 |
bielsnohr
changed the base branch from
gh-pages
to
rework-code-review-section
3 participants
Changes to the code review section, fixes #217
The exercises are going to have the reader be reviewing a pull request identical to this one: thomaskileyukaea/python-intermediate-inflammation#3
One major outstanding issue is how to get the example branch onto the users repo (templates don't copy branches in a way that would work).
It may be easiest to see the changes commit by commit, as I did the structural changes first, before adding new things.
Code Review and Pull Requests In Actionfor the new exerciseRaising a pull requestfor the new exerciseBest Practice for Code Review