#28152: Gettor code refactor with Python Twisted ---------------------------------+------------------------------ Reporter: ilv | Owner: hiro Type: enhancement | Status: needs_review Priority: Very High | Milestone: Component: Applications/GetTor | Version: Severity: Major | Resolution: Keywords: gettor-roadmap | Actual Points: Parent ID: #28232 | Points: Reviewer: phw | Sponsor: Sponsor19 ---------------------------------+------------------------------
Comment (by phw): I had a look at the branch and here are my thoughts: * I find it a bit difficult to understand what some of the commits are trying to accomplish. For example, [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=9f5394e7b32c502f1a0e4d294605996ace50ceaa 9f5394e7] says "Start with main task" but I'm not quite sure what that means. Another example: commit [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=5271b12c105ae394a870936dab9ba634e1af4014 5271b12c] says "Update script". A more descriptive commit message would be "Use Python 3-style format message." Here's a page that provides an excellent overview of crisp and useful commit messages: https://git- scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project * Several commits seem to group unrelated changes together, in a single commit. For example, [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=309e4e384385d45a1db5d0da336837a56472acc8 309e4e38] says "Create script to add links to db" but it also adds and modifies code comments. The modification of comments should be a separate commit. * The last line in [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=61973769729691082d197caa8e07a0a0cd55fa27 61973769] should probably be `print_footer()` and not `print_footer`. * Nitpick: Do we really need spaces in between the `\n` in [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=d4777d66c7cd528bdc4e903de4bea5482eff5d29 d4777d66]? * For substantial changes, it's helpful to provide more than just a one- line summary in the git commit message. For example, I was curious what the purpose of the restructuring in [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=ba19003400fe11a68444bbb585fcdb12a93c100f ba190034] was. Does it make maintenance easier? A short paragraph under the one-line summary wouldn't leave me guessing :) Commit [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=02ae46d0ea3224c041b343832c5f30e8c072c3db 02ae46d0] is another example. It changes several hundred lines of code, yet the commit message only says "Update structure and code". * What's the purpose of the giant TAGS file that was added in commit [https://gitweb.torproject.org/user/hiro/gettor.git/commit/?h=refactoring&id=c953136225ea9f959f94d9257b606973bbad421b c9531362]? Hiro, do you mind cleaning up the commit messages to make them more descriptive? Please let me know if I can help. Also, let me know if there's any code that you want me to pay particular attention to. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/28152#comment:10> 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