BIND 10 #2993: extend ConfigurableClientList::getCachedZoneWriter() to have catch_load_error

[BIND 10 Development]

#2993: extend ConfigurableClientList::getCachedZoneWriter() to have
catch_load_error
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  data source   |                    Milestone:
            Keywords:                |  Sprint-20130709
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  3             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by pselkirk):

 * owner:  pselkirk => muks


Comment:

 In general, it looks good.

 My biggest question is why you don't default the `catch_load_error` to
 `false` (or `False` in the python wrapper). It seems like it would have
 saved a bunch of edits.

 I notice you moved the testdata files out of tests/Makefile into their own
 Makefile. I can see why this might be good, but it's not explained in the
 commit. About half of the testdata directories have their own Makefiles,
 half don't, so there's not a movement one way or the other.

 The documentation for `ConfigurableClientList::getCachedZoneWriter` could
 be a little better:
 {{{
     /// \param catch_load_errors Whether to make the zone writer catch
     /// load errors (see \c ZoneWriter constructor documentation).
 }}}

 I'd rather not send the reader to another method, in another file, to
 figure out what a parameter means, so I prefer the phrasing used in the
 python wrapper:
 {{{
     /// \param catch_load_errors Whether to make the zone writer catch
     /// load errors, or to raise them as exceptions
 }}}

 zonewriter_inc.cc documentation could be more specific.
 {{{
 Depending on how the ZoneWriter was constructed (see catch_load_error\n\
 argument to ConfigurableClientList.get_cached_zone_writer()), in case a\n\
 load error happens, a string with the error message may be returned.
 When\n\
 ZoneWriter is not constructed to do that, in case of a load error, a\n\
 DataSourceError exception is raised. In all other cases, this method\n\
 returns None.\n\
 }}}

 The whole thing is vague and badly worded, but this edit makes it worse. I
 suggest something like the following:
 {{{
 If the zone loads successfully, this method returns None. If the
 ZoneWriter was constructed with the catch_load_error option (see
 ConfigurableClientList.get_cached_zone_writer()), this will return an
 error message string in the case of a load error; if it was created
 without the catch_load_error option, this will raise a DataSourceError
 exception in the case of a load error.
 }}}

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