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

Re: [tor-bugs] #9889 [Atlas]: Add "Tshirt: yes/no" to Atlas and Globe

#9889: Add "Tshirt: yes/no" to Atlas and Globe
-----------------------------+----------------------------
     Reporter:  arma         |      Owner:  hellais
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:
    Component:  Atlas        |    Version:
   Resolution:               |   Keywords:
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+----------------------------

Comment (by meejah):

 hi sreenatha,

 Karsten asked me to take a look at this branch, too. Overall, it looks
 really good! I just have a couple minor suggestions:

 1. For dicts, the recommended way to check for a key is just "if key in
 dict" not "if key in dict.keys()", for example on line 82 of
 https://gitweb.torproject.org/karsten/metrics-
 tasks.git/blob/2f8696e1492c23a2d08562b2dead973ef0d82d2e:/task-9889/tshirt.py
 it would just be: if '3_months' not in response['uptime']

 2. I would recommend using the tool "pep8" to check your syntax. PEP8
 recommends 4-spacing indenting, so you'll have to use "pep8 --ignore=E111"
 to turn that off since this is using 2-space. I think it's more important
 to follow whatever is in the rest of the repository, but in general
 4-space indenting is what most Python code does. I see some warnings for
 mixed tabs and spaces, so you might want to check your editor
 configuration.

 3. It would be nice to accept 1 or more command-line options for relay
 hashes to check, and only ask in case there are none.

 4. The code is well-documented and in general easy to follow.

 5. Besides useful doc-strings, you've included some appropriate inline
 comments, which is good as well!

 If you've never heard of the "requests" library, it is worth checking out
 for things like this -- it's a lot easier to use than urllib2 and checks
 SSL certs by default (which urllib2 does not). I'm not suggesting you
 switch this out in the t-shirt script, this is just for future reference
 :)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9889#comment:12>
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