#26227: Review existing stem.client code ---------------------------+------------------------------ Reporter: dmr | Owner: dmr Type: task | Status: needs_review Priority: Medium | Milestone: Component: Core Tor/Stem | Version: Severity: Normal | Resolution: Keywords: client | Actual Points: Parent ID: | Points: Reviewer: atagar | Sponsor: ---------------------------+------------------------------ Changes (by dmr):
* status: reopened => needs_review Comment: Replying to [comment:10 atagar]: > Hi Dave, sorry about the delay! **Finally** done chewing through these and merged. > > Work looks great! Many thanks for the pull request. Hey Damian! Thanks for the review! Glad you got through it - sorry again that it was such a big chunk to chew on. I'm glancing through the commits you've added in between - they look pretty self-explanatory, but I'll let you know if I have any questions, and will definitely let you know if I have any comments. > Opted to pass on three of the smaller commits... > > * **Commit 8353d83 (Remove redundant content-length checks):** Think I'd prefer to pass on this one. Those conditionals are there so we provide callers with nicer error messages. True, they might not be necessary but not spotting a need to drop 'em. > > * **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. > > Everything else has been merged. Cool! At a glance, it seems like these all make sense to leave out. For [[https://github.com/torproject/stem/pull/1/commits/8353d8399c27901df4412a1e1c8bb57ee3269229|8353d83]] specifically: I guess I didn't think about it from that perspective. I just noticed that several other places relied on the Size `pop()` for the exception behavior; these few seemed inconsistent. In the case of the `NetinfoCell`, the code is practically unreachable, so I'm not sure the check/message really adds much. Either way, I think a good follow-up commit would be to annotate the if check with a comment saying the purpose is for a nicer exception message. (That way it won't look like the code //otherwise wouldn't// check size.) > Feel free to reopen if there's anything I missed here. Hey, in particular I've reopened this for the review comments in this ticket! Namely: * [[comment:4|comment 4]] * [[comment:5|comment 5]] (separate comment, 'cause [[comment:4]] was too long) * [[comment:9|comment 9]] (pending discussions: circ_id allocation; padding, esp. comments in #26228 as called out above) Setting to `needs_review` since that seems to be the most relevant thing here. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26227#comment:13> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs