Dev #9
Merged
Dev #9
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
|
Changes are accepted |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @AdyGCode - I've reviewed your changes and they look great!
General suggestions:
- Ensure that the new chapters and updates are seamlessly integrated with the existing documentation structure for a cohesive learning experience.
- Consider adding more interactive or practical examples in the new chapters to engage readers and reinforce learning.
- Review the transitions between chapters to ensure that the flow of content is logical and that readers can easily navigate the documentation.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
Comment on lines
63
to
+68
| | **Scope of changes** | Typically involve significant changes | Typically involve smaller changes | | ||
| | **Collaboration** | Used to develop ideas in isolation from the main team | Used to develop ideas that the main team is working on | | ||
|
|
||
| Based on SSW.Rules "Do you know when to create a fork vs a branch". | ||
| > Based on SSW.Rules "Do you know when to create a fork vs a branch". | ||
| > | ||
| SSW.Rules. (n.d.). _Git - Do you know when to create a fork vs a branch?_ [online] Available at: https://www.ssw.com.au/rules/fork-vs-branch/ [Accessed 14 Mar. 2024]. |
There was a problem hiding this comment.
suggestion (code_refinement): The addition of a citation for the comparison between forks and branches is a good practice for documentation, ensuring the information is verifiable. However, consider adding a brief summary or key points from the cited article to provide readers with immediate value without needing to leave the page.
Comment on lines
+83
to
+151
| # Pull Requests | ||
|
|
||
| A pull request is a GitHub feature (with similar features in other git hosting). | ||
|
|
||
| A pull request proposes the target repo pulls from a secondary repo or branch. This is in place of pushing to a target repo (“production”) or merging to a production branch. | ||
|
|
||
| Thus “pull request” means: | ||
| > request a pull from your branch instead of pushing to the target repo/branch | ||
| ### Anatomy of a Pull Request | ||
|
|
||
| A pull request has the following features: | ||
| - Has a target repo/branch <=> source repo/branch | ||
| - Has a state: Open, Closed, Merged, Draft | ||
| - Presents a moving Δ between the target and source | ||
| - Contains an area to review and discuss, including in line comments | ||
| - May be a trigger for automated checks and workflows | ||
|
|
||
| #### When combined with Protected Branches | ||
|
|
||
| GitHub allows you to specify that branches with a given name pattern are protected, this means that the branch may insist on: | ||
| - A pull request before merge, | ||
| - At least X reviewers or particular reviewers before merge is permitted | ||
| - and other limitations on the request. | ||
|
|
||
| ## How to Make a Pull Request | ||
|
|
||
| Imagine you created a fork or branch or both, and you think your changes are good enough for the main branch/repo... What should happen next? | ||
|
|
||
| There are two options: | ||
| - Merge and see | ||
| - Propose, discuss/review, then merge | ||
|
|
||
| ### Merge and See | ||
|
|
||
| In “merge and see” your merge your changes and hope for the best. | ||
|
|
||
| This is not as crazy as it sounds since Git is meant to provide us with fail safes... What are the fail safes Git provide you with? | ||
|
|
||
| But: | ||
| - You must have permissions to merge into the repo/branch | ||
| - The code may not meet quality standards and organizational requirements | ||
| - These may be “silent but deadly”... | ||
|
|
||
| A better option could be... | ||
|
|
||
| ### Propose, Discuss/Review and Merge | ||
|
|
||
| Many people and organizations want to review proposed changes prior to merging. This is because the prospect of "bad code" becoming part of the software is not something anyone looks forward to. | ||
|
|
||
| The process of Propose, Review and Merge provides the following advantages: | ||
| - Enhances quality | ||
| - The process ensures that code that gets “mainlined” has been proofed. | ||
| - Encourages collaboration | ||
| - Any changes are not just the responsibility of the contributor of the code. | ||
| - Reviewers and the owners of the project also have a stake in a change. | ||
| - Improves security | ||
| - We can have more authors than committers to the project. | ||
| - These committers act as ‘gatekeepers’ who are the only ones allowed to merge a proposed set of a changes to given repos and branches. | ||
|
|
||
| ## Exercise | ||
|
|
||
| In this repository you will find an exercise that you should download and follow. It is a single Markdown document with all the steps to practice pull requests. Download the [Pull Request Practice Read Me Markdown document](../assets/Pull-Request-Practice.md), and rename it `ReadMe.md`. | ||
|
|
||
| Open this file and follow the instructions. | ||
|
|
||
| To go with this exercise, there is an image, [collaboration.png](../assets/collaboration.png) to be downloaded and used in the exercise. | ||
|
|
||
| [13: Cloning](13-why-hello-dolly.md) | [15: Workflows](18-workflows.md) |
There was a problem hiding this comment.
suggestion (code_refinement): The section on Pull Requests is comprehensive and well-structured, covering the anatomy, creation, and benefits of pull requests. It's a valuable addition for readers unfamiliar with the concept. However, ensure that the transition between sections is smooth and that each subsection is clearly delineated for better readability.
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.
Update, Add and Move Chapters
Update to Chapter 14
Moved workflows chapter to 18-Workflows
Added new chapter on Diffs