-
-
Notifications
You must be signed in to change notification settings - Fork 125
Encourage clean PRs when working on existing projects #1785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,12 +19,39 @@ Some of our standard development practices are _even more important_ when workin | |||||
| We should be _consistent_ with our formatting. We may not have any control over what decisions were made in the past. When we're writing brand new code in a new code, we can choose whether we use two spaces or four. But when we jump into an existing codebase, we should use whatever style it already has. If we send a PR fixing a small bug, and it changes every line in the file, it's really hard to see what changed, and what stayed the same. If we have two people joining a codebase, and one is reformatting every file they touch to use two spaces, and the other is reformatting every file they touch to use two spaces, there are going to be lots of spurious changes, and lots of merge conflicts.\ | ||||||
|
|
||||||
| To approach this, we may need to: | ||||||
|
|
||||||
| * Disable our auto code formatters | ||||||
| * Change the configuration of our auto code formatters (maybe by finding an existing configuration in the repo, or adding one) | ||||||
| * Reformat the whole repo with a consistent configuration in one commit, and then keep it formatted. | ||||||
|
|
||||||
| We should **never** include spurious reformatting changes in the same commit as real code changes. | ||||||
|
|
||||||
| ### Clean PRs | ||||||
|
|
||||||
| We should (almost!) always work from a clean copy of main. When you move from one task to the next, do not keep the changes from the previous task. Often in the real world your last PR will be merged to main before you branch for the new task, so you will have the changes included in the second task's PR, but they are not part of the branch, they are there as part of main. When doing these exercises (or in real world scenarios where your previous branch hasn't been merged) it is important to start from a clean checkout of main. | ||||||
|
|
||||||
| Ideally you'll think about this immediately after you submit the previous PR and before you make any changes. In which case the clean route is | ||||||
|
|
||||||
| ```plain | ||||||
| git switch -c new-branch main | ||||||
| ``` | ||||||
|
|
||||||
| which will remove your committed branch and create a new one based on main. | ||||||
|
|
||||||
| If you've got random uncommitted changes in your local folder, they will not be removed by the above command. So before you do that command you can clean things up: | ||||||
|
|
||||||
| ```plain | ||||||
| git reset --hard origin/main | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a useful "I already made my branch wrong I need to fix it" command, but I read your text above as suggesting this is for creating a new branch? Do we want to suggest: as a default way to make a new branch, and give the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Further commit incoming
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I read this, this risks people resetting their old branch to origin/main rather than "the committed tip of the branch being updated"?
Suggested change
|
||||||
| ``` | ||||||
|
|
||||||
| If you realise (as often happens) too late, and are trying to clean up after you've created your new branch and you have your task2 changes as well as task1 changes, then if they are in different folders you can reset the task1 folder. Check the git history for the last commit to main and note the commit hash. Then do | ||||||
|
|
||||||
| ```plain | ||||||
| git restore --source=COMMIT_HASH --worktree task1folder | ||||||
| ``` | ||||||
|
|
||||||
| Whenever you commit, have a look at the changed files. Check they are only what you need for this task. | ||||||
|
|
||||||
| ### PR Descriptions | ||||||
|
|
||||||
| PR descriptions are even more important when working with Legacy Code. We probably had to do a lot of learning to work out how something works, what was wrong, and how to fix out. The person reviewing our change may not know the codebase very well either. The more detail we can write in our PR descriptions, the more information the next person has when they need to investigate something (and the easier it will be for our reviewer to review our change). | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.