[tor-bugs] #6114 [Stem]: Implement GETCONF parsing in Stem

[Tor Bug Tracker & Wiki]

#6114: Implement GETCONF parsing in Stem
--------------------+-------------------------------------------------------
 Reporter:  neena   |          Owner:  neena         
     Type:  task    |         Status:  needs_revision
 Priority:  normal  |      Milestone:                
Component:  Stem    |        Version:                
 Keywords:          |         Parent:                
   Points:          |   Actualpoints:                
--------------------+-------------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 Just a small bit of feedback since I'm heading to the bus in a few
 minutes.

 > The pop_mapping would return ("DataDirectory", "/tmp/fake") and leave
 "dir" in self._remainder for the next pop. Until some other control
 message's response has similar formatting, I'd like to leave this as it
 is.

 Gotcha. This is actually the second time that I've wanted a 'pop
 everything remaining on the line as a mapping' method (I ran into this
 same issue with GETINFO responses). Maybe we should add a 'remainder' flag
 to pop_mapping() to process everything remaining on the line as the value?

 > I'm not sure what a quoted value would be. Can't find any, yet.

 Did the spec somehow imply that there are quoted values? If so then maybe
 you should ask Nick (he'd, of course, be the most familiar with what it
 might be for).

 > +    Queries the control socket for the values of given configuration
 options. If
 > +    provided a default then that's returned as the if the GETCONF
 option is undefined

 Minor grammatical issue...
 s/returned as the if the GETCONF/returned if the GETCONF

 > +    * :class:`stem.socket.InvalidArguments` the request's arguments are
 invalid

 Lets state the reply types that this exception might be raised by (since
 they're the minority). Also, please make the GetInfo response do this too.

 > +def _getval(dictionary, key):
 > +  try:
 > +    return dictionary[key]
 > +  except KeyError:
 > +    pass

 You want the dictionary's get() method - it's very handy, suppressing
 KeyErrors and returning an optional default value (otherwise returning
 None if the key doesn't exist).

 {{{
 >>> my_dict = {"hello": "world"}
 >>> my_dict.get("hello")
 'world'
 >>> my_dict.get("damian")
 >>> my_dict.get("damian", "blah")
 'blah'
 }}}

 > +def _split_line(line):
 > +  try:
 > +    if '=' in line:
 > +      if line[line.find("=") + 1] == "\"":
 > +        return line.pop_mapping(True)
 > +      else:
 > +        return line.split("=", 1)
 > +    else:
 > +      return (line, None)
 > +  except IndexError:
 > +    return (line[:-1], None)

 I'm pretty sure that this can be replaced by...

 {{{
 if line.is_next_mapping(quoted = False):
   return line.split("=", 1).items()[0] # TODO: make this part of the
 ControlLine?
 elif line.is_next_mapping(quoted = True):
   return line.pop_mapping(True).items()[0]
 else:
   return (line.pop(), None)
 }}}

 Though I suspect that we can do better.

 Looks close to being ready to be pushed. I'll give this a closer look
 after you've replied to this. Also, please rebase onto the current master
 (note that it includes a whitespace checker fix which might uncover issues
 in this change).

 Cheers! -Damian

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6114#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online