[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #28152 [Applications/GetTor]: Gettor code refactor with Python Twisted

#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@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs