search

torproject / stem

Python controller library for Tor
https://stem.torproject.org/
GNU Lesser General Public License v3.0
273 stars 76 forks source link

26227 client code review #1

Closed dmr-x closed 6 years ago

dmr-x commented 6 years ago

Trac ticket

dmr-x commented 6 years ago

All of the commits here are rebased and well squashed/etc. I'd recommend a commit-by-commit review as opposed to a file-by-file review. The commits are fairly orthogonal, but there's a few order dependencies in here.

dmr-x commented 6 years ago

Hmm, the commits in this UI are ordered by date rather than the git parent relationship. So I'd recommend a review in your own tooling, if you're going to walk through in the order I intended to be most logical (rebased commit order, not date order).

dmr-x commented 6 years ago

GitHub doesn't have a way to mark PRs as "Rebased/merged", so this comment will have to do (for now, at least).

Almost everything here was merged. See: https://github.com/torproject/stem/commit/3818cf41cae98ae7558f5002ef3a5152ede5b2fb and https://github.com/torproject/stem/commit/3d3e68c05fbe84f7cf0eff45e376480ec50b48c3

The following were not merged:

  • Commit 819c1a2 (Refactor fixed_cell_len determination into a @staticmethod): No longer necessary due to the LinkProtocol class addition.

  • Commit 79396f5 (Add TODO notes for Cell types without a unit test definition): Eh. Happy to add TODOs but these didn't add much.

Citations for the above: Trac comment 10 and comment 14