BIND 10 #38: review: src/bin/cfgmgr

[BIND 10 Development]

#38: review: src/bin/cfgmgr
--------------------------+-------------------------------------------------
 Reporter:  jreed         |        Owner:  shane                                            
     Type:  task          |       Status:  assigned                                         
 Priority:  blocker       |    Milestone:  02. Running, functional authoritative-only server
Component:  Unclassified  |   Resolution:                                                   
 Keywords:  review        |    Sensitive:  0                                                
--------------------------+-------------------------------------------------
Changes (by mgraff):

  * owner:  mgraff => shane


Comment:

 The code coverage for tests is very good for these.  The remaining few
 seem to be parts that need to be implemented later, with perhaps one
 exception in ccsession check_command().  That might need a test, it looks
 like it's code that was added later perhaps.

 I'd like to see tickets opened for the remaining features (or one long
 ticket for them, or some other list have what is left to do) but I'm ok
 with the "to do later" nature of the unimplemented parts for now.

 Some quick observations:

 module_spec.py and config_data.py both have methods that check the type of
 something using a long if/else statement.  This should probably be
 factored out into a common method (perhaps in the cc code itself) that
 does something like this, or at least into one common function in this
 code.

 In general I dislike the nested if method for argument checking such as is
 done in cfgmgr/_handle_get_config().  I find it much easier to read when
 written as:

   if cmd != None:
     return isc.config.ccsession.create_answer(0, self.config_data)

 as a series of tests, then the functional part of the code once all those
 have passed.  The huge nested if method just means we are going to end up
 indenting off the right side of the page if another item were to be added.
 I know it's a comsci sort of thing to have one entry and one exit into a
 method, but in general I make an exception to that rule for this sort of
 cleanup.


 I know we said 80 columns wasn't a limit, but perhaps some of this can be
 cleaned up by removing the need for as many prefixes?  Can Python import
 the isc.config namespace into this file, so you can use
 ccsession.create_command() rather than the long one?  That would help a
 lot I suspect to make things closer to the somewhat preferred 80 column
 range.


 Some methods are rather long.  For instance, _handle_set_config() is
 around 60 lines long, which means scrolling to look at in many cases.
 Perhaps some of that can be factored out into a few smaller helper
 methods?  This is only one example of this, there are several others that
 are starting to get a bit too long.


 All in all, the code seems good quality, and the test coverage makes me
 smile.

 I'm not certain who gets tagged with this next, but I'll pass it to Shane
 to pass it along.

-- 
Ticket URL: <http://bind10.isc.org/ticket/38#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development