> The changes look great! I see no issues with merging to the master branch.
Great, pushed.
> Sorry about the reStructuredText inconvenience. I actually didn't even think of it -- I just meant to clean up the comments a bit, but I'll definitely be more careful in the future.
No worries. Again, it would usually have been a welcome change. :)
> Once we confirm this with Damian, he will push the changes to master
> in torprojects' repository.
Don't worry about that. The way that open source git projects usually
work is that you make a 'pull request', which simply means saying 'I
have some changes that I would like to share, here they are' (which is
what you've been doing). If the changes look good then I'll rebase
them onto the master branch and push them myself.
Usually only a tiny number of central developers actually have push
access to the master repository.
Ok. Previously I was pulling from 'jacthinman', is 'meganchang' the
github repository where you plan to do most of your future work?
I'd suggest periodically running the following, replacing 'origin'
with whatever you're calling the torproject master remote...
git fetch origin
git rebase origin/master
That way your changes don't fall too far out of date with the current
HEAD (looks like you're currently 29 commits behind).
> +import test.unit.util.proc
Please keep the order of the current imports (the unit/integ test
imports are batched together).
> + test.unit.util.proc.TestProc,
Lets move this test just above the "test.unit.util.system.TestSystem".
The tests are ordered by their dependencies so that the lowest-level
stuff runs first. The reason for that is that if, say, stem.util.enum
breaks then it'll probably break just about everything else so we want
to report those errors first (rather than leave the developer
wondering why something like stem.connection was also broken).
> + prefix_list = sorted(list(line_prefixes))
The extra list wrapper isn't necessary.
>>> sorted((1, 4, 2))
[1, 2, 4]
> Let us know what you think about our unit tests thus far! We also wanted to let you know that we plan on finishing all proc unit tests by Tuesday (June 19), and all proc integration tests by Friday (June 22).
Great! What you have so far looks good, looking forward to seeing the
rest. Be warned that, as Ravi can attest, code reviews generally take
a few iterations. Here's an example...
https://trac.torproject.org/5262
Cheers! -Damian