Skip to content
This repository has been archived by the owner on Jan 3, 2018. It is now read-only.

proposed changes to the code review lesson #517

Merged
merged 4 commits into from
Oct 1, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions novice/extras/02-forking.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
---
layout: lesson
root: ../..
title: Forking a Repository
---

The model shown in the [main lesson](../git/02-collab.html)
in which everyone pushes and pulls from a single repository,
is perfectly usable,
but it will only work if you have write access to the repository.
Sometimes you will want to contribute to someone else's repository
and you won't be able to push your changes to it.
Instead, you can create your own copy of the repository on Github,
push your changes to your copy,
and ask the original author to review and possibly accept your changes
back into the original repository.

Suppose Wolfman wants to be able to make changes to Dracula's project on Github.
Instead of creating a new project,
Wolfman [forks](../../gloss.html#fork) it,
i.e., clones it on GitHub. He does this using the GitHub web interface:

<img src="img/git-fork-ui.png" alt="The Fork Button" />

He then [clones](../../gloss.html#repository-clone) his own GitHub repository,
not Dracula's,
to give himself a desktop copy:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've just used fork and clone quite close to each other, and I think learners might be confused about the difference. Maybe an explicit statement to the effect that if Wolfman clones Dracula's repository to his desktop, he won't be able to push changes to it (unless Dracula gives him permission), but forking makes a new repo owned by Wolfman.

<img src="img/git-forking-01.svg" alt="After Forking on GitHub" />

Now Wolfman can make changes locally, on his machine.
He can make a change to the project
and commit it to his local repository.
Then he can use `git push` to copy those changes to GitHub:

<img src="img/git-forking-02.svg" alt="After Pushing to Fork" />

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest emphasizing here that these changes affect only his own fork, not the original repo, until and unless merging happens.

Once on GitHub
the changes are shared with new potential collaborators.
Wolfman can share his fork
or others can fork his repository with its changes.
He can even share his changes with Dracula.
Dracula can view them
and offer feedback
or accept the changes
and merge them into his own repository,
just as he would review Wolfman's paper before publishing it.

Likewise, Wolfman can review future changes Dracula makes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to illustrate the "upstream" process here? Or maybe further down after showing the "downstream" PR first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is too much for this one lesson. For this modification at least I'd prefer to constrain it to the perspective of the contributor.

and decide to merge them into his fork of the project.
If Dracula decides to remove an important feature,
or rewrite the project,
or delete the project entirely,
Wolfman retains his unmodified copy.
Most importantly,
Dracula does not have to give Wolfman access
to make changes to his repository
in order for Wolfman to create
and share
useful modifications.

This review and merge process
can be done manually
by working in branches,
using `git pull` to get changes from one repository,
and `git push` to apply the changes to another repository.
GitHub has a tool for proposing two repositories share changes:
the [pull request](../../gloss.html#pull-request).

In order to share his changes with Dracula,
Wolfman can create a [pull request](../../gloss.html#pull-request),
which notifies Dracula that Wolfman wants to merge some changes into Wolfman's repository:

<img src="img/git-forking-03.svg" alt="After Creating Pull Request" />

A pull request is a merge waiting to happen.
When Dracula views it online,
he can see and comment on the changes Wolfman wants to make.
Wolfman and Dracula can go through several rounds of discussion,
updating the branch as necessary,
before the pull request is accepted.

Wolfman can update his branch on his fork
and the pull request will automatically update with the changes.
Likewise, Dracula can update his branch
and the pull request will update to reflect the changes.
When Dracula likes the changes
and wants to merge them into his project
he can do so with the click of a button:

<img src="img/github-merge-ui.png" alt="Mergeing a Pull Request" />

If this sounds familiar, it's because it is the way science itself works.
When someone publishes a new method or result,
other scientists can immediately start building on top of it&mdash;essentially,
they can create their own fork of the work and start committing changes to it.
If the first scientist likes the second's work,
she can incorporate those findings into her next paper,
which is analogous to merging a pull request.
If she doesn't,
then it's up to other scientists to decide whose work to build on,
or whether to try to combine both approaches.
137 changes: 0 additions & 137 deletions novice/extras/02-review.md

This file was deleted.

114 changes: 114 additions & 0 deletions novice/extras/03-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
---
layout: lesson
root: ../..
title: Code Review
---

There are many ways to improve the quality of software,
and the most effective of them is [code review](../../gloss.html#code-review).
GitHub uses the [pull request](../../gloss.html#pull-request)
to facilitate code review within and between projects.

Pull requests are a way of proposing a merge,
from one [branch](../../gloss.html#branch) to another.
These branches can be in the same repository
or in different [forks](../../gloss.html#fork) of the same project.
It's an enhancement of the [branching lesson](../extras/01-branching.html)
for working effectively with other people.

Let's revisit Wolfman,
who has made some changes locally
in a branch
that he wants to contribute back
into Dracula's project.
He pushes the branch with the changes to GitHub.
He could push them to the original repository,
or to his fork
if he doesn't have access to the original repository.
Wolfman can then load the repository's page in a web browser,
and open a pull request.
If Wolfman does this within a few minutes,
recently modified branches will be highlighted in the header of the repository
with a button to open a pull request.

<img src="img/github-recently-pushed-branches-ui.png" alt="recently pushed branches are highlighted in the UI" />

If Wolfman wants to open a pull request
from a branch that is no longer highlighted
he can use the link in the repository header
to load an overview of all the branches.

<img src="img/github-summary-header.png" alt="the repository header fields are links" />

From the branch overview, each branch will have a button
that can be used to open a pull request.

<img src="img/github-branch-ui.png" alt="an individual branch row" />

When Dracula views the pull request online,
he will be able to see the changes Wolfman wants to make.
Dracula can leave comments on individual lines of the code,
or make comments on the larger pull request
to give feedback to Wolfman.
Wolfman can make changes to his local code,
and push them to the branch.
The pull request will automatically update to reflect the changes.

The feedback you receive on a pull request
may be extensive, or minimal.
Initially it may be focused on elements of style --
where to put your line breaks,
naming conventions for variables,
or idiomatic code transformations.
Later it could cover your implementation --
how your code is organized
into functions and modules.
Importantly,
the reviewer may ask you to break up your work
into smaller, logical pieces.
This makes your changes easier to review,
easier integrate into the project,
and less likely to cause bugs.

Reviewing code be complex.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing code can be complex.

There are a number of tools that can assist in the task:
software linters are tools to verify that code complies with a style guide,
and automatic tests can ensure no old bugs have resurfaced.
No tool is as effective as having a careful review by a peer, though.
A good heuristic is to look for and scrutinize sources of surprise
as you read through the changes.
Reviews are more difficult than initially writing the code,
and a reviewer's effectiveness diminishes rapidly after the first hour.
Lines of code are a good approximate measure
for how long a patch will take to review.

The process is similar to peer review of scientific publications,
though usually much faster.
In large open source projects
it's very common for a pull request to be updated several times before finally being accepted and merged.
Working this way not only helps maintain the quality of the code,
it is also a very effective way to transfer knowledge.

While Wolfman waits for review to his first modification,
he can continue to work by creating a new branch in his local repository.
From this new branch,
he can make new changes,
push them to GitHub,
and issue a second pull request from that.
This is an important way that Git, Mercurial, and other modern version control systems use branching.
It helps people work together,
but on their own time.
It might take Dracula several days to get around to reviewing Wolfman's changes.
Rather than being stalled until then,
Wolfman can just switch to another branch and work on something else,
then switch back when Dracula's review finally comes in.
When Dracula likes the changes
and wants to merge them into his project
he can do so with the click of a button:

<img src="img/github-merge-ui.png" alt="Mergeing a Pull Request" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mergeing -> Merging


Once the changes in a particular branch have been accepted
and merged into `master` (or some other branch),
Wolfman can delete the branch.
The changes themselves will be preserved where they have been merged.
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ safety.txt setup* waiver.txt
{:class="out"}

> #### Necessary But Not Sufficient
>
>
> The fact that something is marked as executable
> doesn't actually mean it contains a program of some kind.
> We could easily mark this HTML file as executable
Expand Down Expand Up @@ -126,8 +126,8 @@ On the right side, we have the files' names.
Next to them,
moving left,
are the times and dates they were last modified.
Backup systems and other tools use this information in a variety of ways,
but you can use it to tell when you (or anyone else with permission)
Backup systems and other tools use this information in a variety of ways,
but you can use it to tell when you (or anyone else with permission)
last changed a file.

Next to the modification time is the file's size in bytes
Expand Down Expand Up @@ -165,7 +165,7 @@ $ ls -l final.grd
~~~
{:class="out"}

Whoops: everyone in the world can read it&mdash;and what's worse,
Whoops: everyone in the world can read it&mdash;and what's worse,
modify it!
(They could also try to run the grades file as a program,
which would almost certainly not work.)
Expand All @@ -177,7 +177,7 @@ $ chmod u=rw final.grd
~~~
{:class="in"}

The 'u' signals that we're changing the privileges
The 'u' signals that we're changing the privileges
of the user (i.e., the file's owner),
and `rw` is the new set of permissions.
A quick `ls -l` shows us that it worked,
Expand Down
Loading