Re: BIND 10 #322: Process name in Python

[BIND 10 Development]

#322: Process name　in Python
-------------------------------+--------------------------------------------
      Reporter:  fujiwara      |        Owner:  vorner   
          Type:  enhancement   |       Status:  reviewing
      Priority:  major         |    Milestone:           
     Component:  Unclassified  |   Resolution:           
      Keywords:                |    Sensitive:  0        
Estimatedhours:  0.0           |        Hours:  0        
      Billable:  1             |   Totalhours:  0        
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by jinmei):

  * owner:  UnAssigned => vorner


Comment:

 '''general comments'''
  - please provide suggested ChangeLog entry.
  - I'm afraid the library name "bind10" maybe misleading because it sounds
 like something related to the bind10 process.  I have no specific idea
 about a better name, but something like "util"?
  - Likewise, I suspect "rename" is too generic.  We see in the program
 code:
 {{{
 import isc.bind10.rename
 isc.bind10.rename.rename()
 }}}
  and it's not obvious that it renames the process title.

 '''configure.ac'''
  - I'd clarify "--disable-setproctitle-check" is about a python library,
 either or both in the option name and option description.

 '''rename.py'''
  - please add the copyright notice
  - please describe this is a wrapper interface to the setproctitle
 library.
  - it strips path to the program name by default, so we'll see "bind10"
 instead of "/usr/local/sbin/bind10", etc.  Personally, I'd like to know
 the latter, because I may have multiple different versions installed in
 different places and I'd like to distinguish them.

 '''bind10/tests/Makefile.am'''
  - you don't need the LIBRARY_PATH hack for this test.  it's necessary
 only for those that require our C++ loadable modules.

 '''tests/rename.py'''
  - we normally call files for tests "xxx_test.py" (it's redundant because
 the directory name indicates it's related to tests.  but on the other hand
 it helps to separate real test code and other helper files).  it's not a
 strong requirement, but unless you strongly want to keep this naming I'd
 suggest follow the convention for consistency.
  - some methods are named "__xxx".  in other python code we generally seem
 to use "_xxx".  I don't know which one is the standard way, but it's
 better to be consistent.
  - __scan(): variable repl doesn't seem to be used.
  - __scan(): according to the manual assert_ is deprecated by
 assertTrue().

 ''' tests/rename.py:TestRename.test_calls()'''
  - I respect the paranoia:-), and I actually like it, but I suspect this
 is beyond the scope of unit tests.
  - more practically, this (at least currently) doesn't work if you try
 'make check' for a vanilla tree.  so, I'd suggest this test is separated
 as part of some kind of "distcheck" level check.
  - the regular expression "lines" is difficult to understand.  Why \\w
 instead of \w?  same for \\s, and what does \\\\\\n mean?  If it can be
 simplified, please do so.  And in any case, I think this requires a
 descriptive comment.
  - os.walk(): I don't like to hardcode the path for the 1st argument.  I'd
 use top_srcdir and have the configure script replace it.
  - os.walk(): do we really need to follow symbolic links (despite the risk
 of causing an infinite loop)?
  - what's the purpose of second variable (_) in the for ... findall idiom?
 e.g.
 {{{
 +                for (var, _) in lines.findall(makefile):
 }}}
  it seems to me it could simply be for var in lines.findall(makefile)

-- 
Ticket URL: <https://bind10.isc.org/ticket/322#comment:3>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development