Skip to content

Add tests with additional clean up#61

Closed
pwoodworth wants to merge 2 commits into
mpetazzoni:masterfrom
pwoodworth:addTests
Closed

Add tests with additional clean up#61
pwoodworth wants to merge 2 commits into
mpetazzoni:masterfrom
pwoodworth:addTests

Conversation

@pwoodworth

Copy link
Copy Markdown
Contributor

This is a cleaner attempt at the closed https://github.com/turn/ttorrent/pull/58 in a new pull request to avoid losing more comment history.

This includes the tests from #46, cleaned up to minimize unnecessary changes to existing code, and reformat the test code to match existing project code style.

@pwoodworth

Copy link
Copy Markdown
Contributor Author

Sorry for the thrash with a new pull request, but I was dismayed to lose your comments on the code in the last pull request even though I believe I addressed them all. Anyway I won't touch this branch again until you've commented (or just closed) this pull request. Thanks for taking the time to review it.

@mpetazzoni

Copy link
Copy Markdown
Owner

Are you sure that second commit (the merge commit) is supposed to be there? It seems like it contains a lot of things I already have merged into master. Shouldn't you have rebased Sergey's work on top of master before proposing this pull request?

@pwoodworth

Copy link
Copy Markdown
Contributor Author

@mpetazzoni Yes the merge out from master is correct, in that when this branch gets merged to master it will only result in the changes seen in this pull request. It would of course be slightly cleaner to rebase Sergey's work but then they would show up as my changes rather than Sergey's and again I'm not comfortable expunging him from the history. To get a cleaner history at this point would require Sergey to post a branch rebased on master and incorporating my cleanup which I'm fine with, but isn't under my control.

@pwoodworth

Copy link
Copy Markdown
Contributor Author

Okay, I think I might've figured out how to get a better history, without losing Sergey's credit and without merging in/out from master on the PR branch. To do so, I'll be posting yet another new pull request and closing this one (rather than force pushing and losing comment history again).

@pwoodworth pwoodworth closed this Sep 3, 2013
@pwoodworth pwoodworth deleted the addTests branch September 3, 2013 19:33
@pwoodworth pwoodworth mentioned this pull request Sep 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants