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

Conversation

lonnen
Copy link
Contributor

@lonnen lonnen commented Jun 22, 2014

edit: ok, this is ready for review

I have some ideas for improving novice/extras/02-review.md and would like some feedback before I start working. I propose the following:

  1. split out a lesson on creating and maintaining your own fork, to be taught before the code review lesson
  2. enhance the existing code review lesson to teach how to create pull requests between branches within one repo and across forks
  3. flush out a little bit of technique for how to review code

I'm not sure if point 1 needs to be a separate lesson or if this can be structured together as one. Point 2 supports a simplified branching model I see at some companies and expect to see at some labs where individuals all work on branches of the parent repo.

@gvwilson
Copy link
Contributor

Our novice lessons don't introduce branching - it's too much on top of everything else for people who've never seen version control before - but the rest looks good to me.

@lonnen
Copy link
Contributor Author

lonnen commented May 29, 2014

I had this in mind in the extras section, where 02-review.md is currently and 01-branching.md is the first lesson. Did you want it in the main novice curriculum?

@gvwilson
Copy link
Contributor

Putting it in extras makes sense - please go ahead with your plan, and
thanks.

@lonnen
Copy link
Contributor Author

lonnen commented Jun 22, 2014

In progress. The new 02-forking lesson is ready for feedback.

still todo:

  • remove duplicate material with 03-review
  • enhance 03-review with details about pull requests from branches in the same repo
  • enhance 03-review with some advice on how to review code

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.

lonnen added a commit to lonnen/bc that referenced this pull request Jul 15, 2014
to address comment swcarpentry#1 on pull request swcarpentry#517, this makes explicit that you won't
always be able to push directly to repositories other people have made.
lonnen added a commit to lonnen/bc that referenced this pull request Jul 23, 2014
to address comment swcarpentry#1 on pull request swcarpentry#517, this makes explicit that you won't
always be able to push directly to repositories other people have made.
@lonnen
Copy link
Contributor Author

lonnen commented Aug 13, 2014

ready for review + merge

lonnen added 4 commits August 13, 2014 15:27
Create an empty lesson on forking and bump the other lesson numbers up accordingly.
Add the first draft of a new lesson about forking. The lesson covers creating a fork, creating pull requests, and reasons for using a fork instead of working off of one repo. Leaves out the details of code review.

This encompasses most of the material that is in lesson 03. The plan is to rewrite 03 to focus on pull requests and code review. It should cover using pull requests from different branches in the same repo as well as between forks.
to address comment swcarpentry#1 on pull request swcarpentry#517, this makes explicit that you won't
always be able to push directly to repositories other people have made.
remove redundant curriculum with 02-forking, add some additional images to help guide creating a pull request, and add an abstract guide to performing code review
@gvwilson
Copy link
Contributor

Can the Git topic maintainers please decide whether to merge this one, and if so, resolve the conflicts and push the button?

@lonnen
Copy link
Contributor Author

lonnen commented Sep 29, 2014

I took a stab at rebaseing this myself locally but it looks like 02-collab has had some work, at least, that conflicts. Since I effectively split that into two or three separate articles it will take some work to resolve conflicts and I don't have any cycles to spare. Sorry.

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.

jiffyclub added a commit to jiffyclub/bc that referenced this pull request Oct 1, 2014
Creates a new lesson on forking on GitHub and revises
the code review lesson.

Closes swcarpentry#517.
@jiffyclub jiffyclub merged commit 89ee0b8 into swcarpentry:master Oct 1, 2014
jiffyclub added a commit that referenced this pull request Oct 1, 2014
Adds a new lesson about forking on GitHub
and revises the code review lesson.

Closes #517.
@jiffyclub
Copy link
Contributor

I've manually merged and pushed this, thanks for adding the lesson.

It occurred to me while reviewing this that there don't seem to be any actual images or descriptions of the mechanics of code review on GitHub. For example that you can leave inline comments by clicking to the "Files" tab and then on lines, or leave comments on a commit by going to the commit view. Something the flesh out in the future.

@wking
Copy link
Contributor

wking commented Oct 1, 2014

On Tue, Sep 30, 2014 at 09:43:21PM -0700, Matt Davis wrote:

For example that you can leave inline comments by clicking to the
"Files" tab and then on lines, or leave comments on a commit by
going to the commit view.

Or just link out to here 1 or the more generic 2.

@lonnen lonnen deleted the improve-code-review branch October 1, 2014 21:08
@lonnen
Copy link
Contributor Author

lonnen commented Oct 1, 2014

Thank you, all involved. It's good to have it landed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants