Re: [tor-bugs] #9376 [Pluggable transport]: subprocess management module for pyptlib

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#9376: subprocess management module for pyptlib
---------------------------------+------------------------------------------
 Reporter:  infinity0            |          Owner:  asn           
     Type:  enhancement          |         Status:  needs_revision
 Priority:  normal               |      Milestone:                
Component:  Pluggable transport  |        Version:                
 Keywords:  pyptlib              |         Parent:                
   Points:                       |   Actualpoints:                
---------------------------------+------------------------------------------

Comment(by infinity0):

 Replying to [comment:9 asn]:
 > Looks good. Two things:
 >
 > - Nice tests! Any chance they could get some small documentation? For
 example, what is `test_killal_kill` supposed to do, and how does it do it?
 It doesn't send any signals or anything.
 >
 > - You didn't document the `_Popen_defaults =
 zip(a.args[-len(a.defaults):],a.defaults); del a`. Can you document it?
 >

 Fixed.

 > Some further comments. If you don't have time to do these, I can fix
 them myself before merging:
 >
 > - Hm. It seems like you moved all the `util.py` file to
 `util/__init__.py`. I don't like code in `__init__.py` except if there is
 a reason. Is there a reason for it?
 >

 Python needs __init__.py to find the submodules (here, subproc.py). If you
 don't want code in __init__.py, you can move everything to say,
 util/util.py then add aliases in util/__init__.py (to avoid breaking
 previous clients), but I don't think it's worth the effort here. I can add
 a note for future developers though. Actually util.checkClientMode() could
 be moved into config.py already, but perhaps it would be cleaner to get
 rid of it altogether. I don't know how many others use it.

 > - I'm not really used to the following idiom (wrt proc_is_alive()):
 > {{{
 > if mswindows:
 >     def something():
 >         XXX
 > else:
 >     def something():
 >         YYY
 > }}}
 > I prefer the following:
 > {{{
 > def something_impl_windows():
 >        XXX
 >
 > def something_impl():
 >        YYY
 >
 > if mswindows:
 >      something = something_impl_windows
 > else:
 >      soemthing = something_impl
 > }}}

 Unfortunately we do some platform-specific imports as well so it's not as
 simple as that. Either I would have to do

 {{{
 try:
   import windows-specific
 except ImportError
   pass

 try:
   import others
 except ImportError
   pass

 # then what you said
 }}}

 or

 {{{
 if mswindows:
   import windows-specific
 else:
   import others

 # then what you said
 }}}

 neither of which seem that much cleaner.

 If it helps, python's subprocess.py do a similar thing to me and even do a
 conditional "def" in the middle of the Popen class itself. :p

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