Rework of code review episode #286
Conversation
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.
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
…/python-intermediate-development into code-review-review
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
…-course-project Switch to using fork of project repository instead of copying a template
Initial review and redo of screenshots
Co-authored-by: Steve Crouch <s.crouch@software.ac.uk>
…oject Switch to using fork of project repository instead of copying a template
@anenadic as you can see from our most recent sprint notes we discussed what to do with the inflammation project repository, and agreed that we would create a new repository instead of modifying the existing one. The advantage of this is that it won't break any forks of our material that don't keep up-to-date with the most recent changes. |
Hi! @steve-crouch and I had another round of conversations around this. The solution in the end was to keep the original inflammation repository with the template option but to require learners to fork instead. This way, it does not break anything for anyone (older lesson forks can use the template option and ignore the new feature-std branch) and the latest version of the lesson is supported as well. We decided against creating a new inflammation repo (as we would have to maintain which lesson version is using which inflammation repo through some global parameter - not the wort thing but still) and we would probably have to release a new inflammation repo each time we release the next version of the lesson (just to keep the versions in sync, even if no new changes are introduced to the inflammation repo). So, we did not want to clutter the Incubator with these multiple inflammation repos. We also decided that we will notify all lesson fork owners of the change and give them enough warning that the template version of the inflammation repo might be switched off some time in the distant future (at the same time inviting them to join our lessons's mailing list (not the maintainers' one)). I had a chat with @tobyhodges about this as well earlier this week and he confirmed this line of thinking is the best and causes the least amout of disruption and additional work. |
This PR is a merge of two related PRs: #282 and #285 to rework episode on code review and contributed by @thomaskileyukaea 🎉 (and @bielsnohr). It requires an additional change on the inflammation project repository (to add an additional std-dev-feature branch) and this merge will be done at the same time as that other one.
Fixes #217 and #249.