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:
                Type:  task          |  pselkirk
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130709
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => pselkirk


Comment:

 Hi Paul

 Replying to [comment:7 pselkirk]:
 > 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.

 It matches the C++ arguments exactly. We can have it optional or
 mandatory consistently in both the C++ and Python code. I feel it's
 better to specify this argument explicitly as it influences exception
 behavior.

 > 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.

 In hindsight, I should have broken that commit into separate bits, where
 one commit just does the `Makefile.am` creation. That commit indeed looks
 cluttered.

 I'll explain here in the ticket.  IMO directories where files have to be
 built or dist'ed are better off having their own Makefile.am's as it's
 easier to move content such as test data and match them with the
 makefile rules. If we don't do this for some directories in our tree, we
 ought to eventually for consistency.

 > 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
 > }}}

 We follow DRY (don't-repeat-yourself) everywhere in the class
 documentation and avoid copies of behavioral docs.

 The Python documentation is different because its `ZoneWriter` cannot be
 constructed and doesn't have any associated constructor documentation.

 > 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.
 > }}}

 This documentation has been modified as you suggested.

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