search

LearnProgramming / percival

An IRC bot designed to help the LPMCers track quotes, log channels, and all sorts of things
22 stars 8 forks source link

Name changer and channel changer #13

Closed colwem closed 12 years ago

colwem commented 12 years ago

What am I supposed to write in the title and this section.

jfredett commented 12 years ago

Apologies for the delay,

Wrt to this pull, the following things are needed:

1) We need to rebase this on master (it's updated since the original pull)

2) The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3) the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

-- Generally, the commits should be 'logical' -- eg, they should group related changes together, and should avoid redundant/unnecessary changes (eg, add debugs, remove debugs, add comments, remove comments, etc) -- Ideally those things don't get commited in the first place, but in a pinch, they can be rebased away.

I know those things seem nitpicky, but git history is a powerful tool in long term maintainership of a repo. The code itself seems good. The tests seem to fail for me, but not yours. I chalk that up to my brittle tests and not your stuff. As for rebasing/reworking, it's really just those bottom three commits (starting at 09031da) that seem to be real offenders.

After that stuff gets fixed and the pull is updated, I'll merge this. @SirSkidmore since you're pull is included in this one, I'm going to just merge this one (since it makes my job easier), Will also post this on your PR before closing it.

travisbot commented 12 years ago

This pull request passes (merged 40e14b5c into b33d958d).

travisbot commented 12 years ago

This pull request passes (merged e5fdd379 into b33d958d).

jfredett commented 12 years ago

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

colwem commented 12 years ago

So do I make a new commit for these small changes or should I fit them into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

— Reply to this email directly or view it on GitHubhttps://github.com/LearnProgramming/percival/pull/13#issuecomment-7712858.

travisbot commented 12 years ago

This pull request passes (merged 2711b9fa into b33d958d).

colwem commented 12 years ago

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

— Reply to this email directly or view it on GitHubhttps://github.com/LearnProgramming/percival/pull/13#issuecomment-7712858.

jfredett commented 12 years ago

:octocat: Approved!