BIND 10 #284: Not every 'data' parsing is done with json

[BIND 10 Development]

#284: Not every 'data' parsing is done with json
-------------------------------+--------------------------------------------
      Reporter:  jelte         |        Owner:  jinmei                     
          Type:  defect        |       Status:  reviewing                  
      Priority:  minor         |    Milestone:  06. 4th Incremental Release
     Component:  Unclassified  |   Resolution:                             
      Keywords:                |    Sensitive:  0                          
Estimatedhours:                |        Hours:                             
      Billable:  1             |   Totalhours:                             
      Internal:  0             |  
-------------------------------+--------------------------------------------
Changes (by jelte):

  * owner:  jelte => jinmei


Comment:

 >  - general: I'm not sure why you changed single quote to double-quote
 (although I have no objection to that).  Is it just some style change or
 something inevitable for this ticket?
 >  - general: I don't know if s/false/False/ is necessary, but I trust
 >    you on this point:-)


 Both of these are also because the python json parser is pretty strict in
 this regard, it does not accept single quotes, nor False and True.

 >  - same for s/ast.literal_eval/json.loads/ (about this is correct)


 Well, that's for consistency, literal_eval parses python structures,
 json.loads parses JSON. They look a lot alike but are subtly different (as
 you've found out in your first 2 comments :) ). And since we 'officially'
 use JSON for this, even though our c++ json parser can handle 'python
 representation' (i.e. single quotes, False, etc.), for consistency it's
 better to use the JSON parser.

 >  - cmdctl_test.py: isn't it better to imprt sys at the beginning of the
 file?
 >  - cfgmgr.py: I didn't understand why pprints were commented out.  If
 they are not necessary anymore why not just clean them up?
 >  - config/module_spec.py
 >   - better to import at the beginning of the file?


 all three ack, updating

 >   - not necessarily for this changeset, but I personally see read(-1)
 >     rather confusing.  Wouldn't it be clearer to pass nothing to
 >     read()?  Or at the risk of looking redundant we might define a
 >     constant such as READ_EVERYTHING = -1 and pass it to read()


 oh, yes, no argument is ok too, and more clear

 >   - why aren't exceptions in the 'if' case caught?  Also, is it
 >     okay/safe to ignore the error and blindly initialize module_spec
 >     with an empty config? (shouldn't we rather fail with reporting an
 >     error?)


 technically this case is still caught (it'll throw an exception later if
 it's empty) but the error then is misleading. Catching and throwing right
 away.

 >   - is it a good idea to catch all exceptions here?  I wouldn't
 >     necessarily object to all catch-all catches, but in general we
 >     should be careful about that, and in (rare) cases it's justified
 >     I'd like to see an explicit comment about why we need catch
 >     everything here and why it's justified.


 No, that's an oversight (initially there because the documentation isn't
 clear on what exceptions can be thrown). Changing to the right one.

 >   - we probably need test cases when json.loads fails.


 ack

 Updates done in r2532, please verify.

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