Rework code review section #279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As per #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
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 the only way explained in this episode, and you'd have to go out of your way to do it any other way.
While this is generally good advice, some open source repositories will require force-pushing to keep commit history tidy. Further, this is good advice in general, not related to pull requests, and most people won't have come across the idea of force-pushing anway, so no need to confuse them. It also isn't accurate, it can't corrupt the pull request, it can just get the reviewer tied in a bit of a knot if they don't know what they are doing and pulled the code locally.
Added a line about WSL not being supported
A callout on VS Code and VS Code episode improvements
Expanded diagram to include git pull using Mermaid JS tool.
Reformatted the authors file and added Matt to the list
Rework code review section
Some minor grammatical improvements Co-authored-by: Aleksandra Nenadic <[email protected]>
The max variable should stay named as is so that an overload warning is spit out by the linters later
…hanges Accumulated Changes from UKAEA Pilots
Co-authored-by: Matthew <[email protected]>
Co-authored-by: Matthew <[email protected]>
Replacing git checkout with git switch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For discussions see #217 for initial issue and PR #216. Discussed with Aleks and approved.