BIND 10 #2853: Python wrapper of data source extensions

[BIND 10 Development]

#2853: Python wrapper of data source extensions
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:17 vorner]:
 > Hello
 >
 > First, where is the `catch_load_error` thing? I can find it in
 documentation, but not in the code. Is it planned in future ticket?

 This is an argument to the C++ `ZoneWriter` constructor. On purpose,
 it's not possible to directly construct a `ZoneWriter` using the
 Python API. But the `ZoneWriter` API documentation indicates how the
 methods behave for the corresponding constructor arguments.

 > About the current code:
 >
 > This bit seems odd. Does it mean that when I get an empty list in C++, I
 get `None` in python, so I can't really write simple iteration with `for`
 and need to check for `None` first?
 > {{{#!c++
 >     try {
 >         const std::vector<DataSourceStatus> status =
 self->cppobj->getStatus();
 >         if (status.empty()) {
 >             Py_RETURN_NONE;
 >         }
 > }}}

 Nod.. what you say makes sense. This has been updated to return an empty
 list in case an empty vector is returned by `getStatus()`.

 > Is it really interesting to the user of the API that a function is a
 wrapper, that it is mentioned as the first thing in the documentation
 (with `get_cached_zone_writer` and `get_status`)? I believe that's more
 like an internal thing nobody from outside cares.

 I have removed the text which said it was a wrapper. I was following
 `configure()` method's documentation when writing that comment. The
 Python documentation has been updated now.

 > Would this check be possible as some kind of decorator? Because it seems
 awkward to claim the test as passed when we skip it in fact:
 > {{{#!python
 >         """
 >         Test find on a mapped segment.
 >         """
 >         if os.environ['HAVE_SHARED_MEMORY'] != 'yes':
 >             return
 > }}}

 I didn't know about such a use of decorator in unit tests. Thank you for
 pointing it out. I have replaced it with a decorator.

 > Should this be `True` instead of `false`?
 > {{{#!python
 >   DataSourceError load related error (not thrown if constructed with\n\
 >              catch_load_error being false).\n\
 > \n\
 > }}}

 Nod. I have updated the Doxygen source of this comment too in
 `zone_writer.h`.

 > Should there be tests for the zone writer's methods throwing?

 Tests have been added.

 > I didn't review the new utility script (`doxygen2pydoc`) thoroughly, I
 don't think it is needed. But I noticed an oddity anyway. There's an `r`
 just before the top-level doxy comment. Is that on purpose, and if so,
 what does that mean?
 >
 > {{{#!python
 > r"""
 > }}}

 This indicates a "raw" string. It means that the backspace character is
 not considered as an escape character and is included as-is, etc.


 Hi Jinmei

 Replying to [comment:18 jinmei]:
 > noticed while trying to use it for #2854: docstring for some
 > `ConfigurableClientList` is not helpful:
 >
 > {{{
 >  |  get_status(...)
 >  |      get_status() -> list
 >  |
 >  |      Wrapper around C++ ConfigurableClientList::getStatus
 >  |
 >  |      This returns a list of tuples, each containing the status of a
 data source client.
 >  |
 >  |  reset_memory_segment(...)
 >  |      reset_memory_segment(datasrc_name, mode, config_params) -> None
 >  |
 >  |      Wrapper around C++ ConfigurableClientList::resetMemorySegment
 >  |
 >  |      This resets the zone table segment for a datasource with a new
 >  |      memory segment.
 >  |
 >  |      Parameters:
 >  |        datasrc_name      The name of the data source whose segment to
 reset.  mode              The open mode for the new memory segment.
 config_params     The configuration for the new memory segment, as a JSON
 encoded string.
 > }}}
 >
 > This doesn't even explain the number of entries of the tuples.  For
 > Python programmers using this API, we need an equivalent to the
 > description of `DataSourceStatus`.

 Documentation describing the Python method behavior has been added now.

 > Also, it's not really useful for Python programmers to know it's a
 > wrapper for some C++ methods; all they need to know is how
 > specifically they can use these methods in their Python programs (I
 > wouldn't necessarily insist they be removed, if they are mentioned as
 > information for someone extending the wrapper interface itself, but
 > right now it looks very awkward in that it doesn't provide more
 > important description while providing such bonus information).

 I have removed the text which said it was a wrapper. I was following
 `configure()` method's documentation when writing that comment. In any
 case, the documentation should be better now.

 > The description of reset_memory_segment is slightly better, but at
 > least its format isn't pretty.

 The format should be prettier now. :)

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