search

hotsh / rstat.us

Simple microblogging network based on the ostatus protocol.
http://rstat.us/
Other
722 stars 215 forks source link

Fixing Issue 698 #708

Closed gavinlaking closed 11 years ago

gavinlaking commented 11 years ago

Massive refactor and fix of Issue 698.

wilkie commented 11 years ago

There is a lot here, and a fair bit of it is a tad messy. Let's just pull out what is necessary to address the issue (that's my priority) and leave the refactoring work for a new pull request. If you can, split up the refactoring appropriately so that it is easier to review.

Thanks. :)

steveklabnik commented 11 years ago

:+1:. Please split this up into smaller pulls. I'll leave this one open so you can leave just the bugfix.

carols10cents commented 11 years ago

I'mma take a look at this.

carols10cents commented 11 years ago

Hey @gavinlaking, I love a lot of the work you've done in here. I especially love the refactorings you've done like revealing intent by renaming variables, and I love that you mentioned that that's what you did in the commit message.

I'm working on splitting this PR out into separate pull requests that can be reviewed and merged independently of each other. This also involves splitting the big commits into separate commits that can be reviewed and merged independently of each other. One hint that a commit may be too big is a bulleted list of things that were changed in the commit message :)

In general, to make a clean, understandable commit log that is useful in the future for being able to tell exactly what happened in each commit and why without needing the context of the whole pull request.

For example, it's better to fix tests in the same commit as they're broken, since a commit in isolation that says "fix tests" doesn't give any indication of why they were broken, which previous change broke them, etc. It also helps when refactoring to have the system in a broken state for as short a time as possible.

The tools I'm using to separate out these commits are mostly git's interactive rebasing and add --patch mode. I use these ALLLLL the time and I'd really suggest learning them to get to the next level of git-fu and making super awesome commits to go with your super awesome code improvements :) This article about interactive rebase is pretty good, and this screencast is great at explaining add --patch.

I'm going to keep you as the author of these commits since I definitely want you to get the credit for all these, but I am making significant changes to how they're broken up (as well as choosing some things over others, for example, I liked your first refactoring of ConvertsSubscriberToFeedData better than your later refactoring), so I'd love for you to review the pull requests I'm going to enter to make sure I've captured your intent correctly, if you have time.

Thank you so so so much, we needed this :heart: :heart: :heart:

carols10cents commented 11 years ago

Ok, I think I have everything separated out now:

https://github.com/hotsh/rstat.us/pull/720 https://github.com/hotsh/rstat.us/pull/716 https://github.com/hotsh/rstat.us/pull/715

There are a few things that I didn't keep or that I changed, but most of this was really awesome :) Thank you!!

And I've added you to the team of committers, please read what that means to us here: https://github.com/hotsh/rstat.us#becoming-a-committer

And I'll have you up on the about page in just a bit.

Thank you thank you!

carols10cents commented 11 years ago

derp ignore me! This wasn't your first commit! haha.