BIND 10 #2254: bindctl tab completion confusing output

[BIND 10 Development]

#2254: bindctl tab completion confusing output
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:
                  Component:  bind-  |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Since the branch has been heavily revised, I've basically given a full
 review from the branch point, rather than checking the diff from the
 previous version.  So, below, I may be repeating some of the points in
 the previous comments.

 '''general'''

 - I made a few editorial and suggested documentation updates and
   committed them.
 - I really hope the next task of the config/command stuff is the more
   fundamental overhaul, but if we still need to pick up some bandaid
   bug fix tasks, I'd suggest "no more piggy back" (i.e., no more "also
   wanted to fix another config issue in this ticket").  Due to the
   complexity of the config stuff, handling multiple things at the same
   time can easily blow your brain stack.
 - As noted, we should close #2289 with this ticket.  I'm not sure
   about #2290, but if it's still open, please don't piggy back it to
   this ticket; defer it to #2290 itself (or even after the overhaul:-)

 '''bindcmd.py'''

 - I now understand removing '[' from completer_delims solves the
   duplicate completion I noticed.  But now I wonder whether whitespace
   variants as a delimiter make sense:
 {{{#!python
     # Only consider whitespace as word boundaries
     readline.set_completer_delims(' \t\n')
 }}}
   is there a case where a problem happens if we completely disable all
   delimiters?
 - As for set_completer_delims, since the effect is not so obvious, I
   think it's worth keeping some of the removed comments, like the
   reference to #1345 (and maybe this ticket too)
 {{{#!diff
 -    # This is a fix for the problem described in
 -    # http://bind10.isc.org/ticket/1345
 ...
 }}}
 - `_get_identifier_startswith`: I suggest this for better readability:
 {{{#!python
         list = self.config_data.get_config_item_list(
                         id_text.rpartition("/")[0], recurse=True)
 }}}
   (clarifying the parameter name for True)
 - `_get_identifier_startswith`: I suggest describing the type of
   id_text (string?) and the expected format in docstring not to make
   the reader of the code get lost.  some examples may also help.
   same for `_cmd_has_identifier_param`. (Actually I committed my
   suggested text for the latter at fa10a89).

 - complete(): probably partly because of the fundamental complexity of
   the config/command stuff, it's quite difficult to understand what
   the diff tries to do:
 {{{#!diff
 -                    if cmd.module == CONFIG_MODULE_NAME:
 ...
 +                    if self._cmd_has_identifier_param(cmd):
 }}}
   I think I'm getting the idea, but could you explain in plain word
   what the previous version tries to do, what was wrong with it, and
   how the new version solves that?
 - complete(): in the new code it seems possible to completely separate
   hints construction:
 {{{#!python
                 elif self._cmd_has_identifier_param(cmd):
                     # For tab-completion of identifiers, replace hardcoded
                     # hints with hints derived from the config data
                     id_text = self.location + "/" + cur_line.rpartition("
 ")[2]
                     hints = self._get_identifier_startswith(id_text)
                 else:
                     hints = self._get_param_startswith(cmd.module,
 cmd.command,
                                                        text)
 }}}
  because the `elif` case doesn't need the result of
  `_get_param_startswith()`.  and I think we need some clarification
   comments explaining what this if-elif-else block tries to do in
   general, not only for the "For tab-completion of identifiers" part.

 - I'd suggest adding doc for complete(), with the type and meaning of
   parameters and possible examples.
 - complete(): it's difficult to understand this line:
 {{{#!python
                         id_text = self.location + "/" +\
                             cur_line.rpartition(" ")[2]
 }}}
   Why slash?  Why white space?  What's expected for location and
   cur_len?  Please either write (or specify exist) a test or leave
   comment what it does with an example.

 '''bindctl_test.py'''

 - overloading of variable 'cmd' is confusing: first used as
   `CommandInfo`, then  as `BindCmdParse`.  I'd rename the second
   cmd_parser (I'd even class name to `BindCmdParser`)
 - minor point: the variable name 'tool' sound confusing to me.

 '''config_data.py'''

 - I still don't get it, so: Is this related to any issues of this
   ticket?
 {{{#!diff
 -        else:
 +        # ignore any
 +        elif not spec_part_is_any(spec):
 }}}

 - changes to MultiConfigData.find_spec_part() don't seem to be
   directly tested.

 - `_get_list_items`: to help understand it, I'd like to see docstring
   that explains the type of item_name and its expected form.
 {{{#!python
     def _get_list_items(self, item_name):
         """This method is used in get_config_item_list, to add list
            indices and named_set names to the completion list. If
            the given item_name is for a list or named_set, it'll
            return a list of those (appended to item_name), otherwise
            the list will only contain the item_name itself.

            Parameter:
            item_name (TYPE): description, example

            Return:
            XXX

            """
 }}}

 - `_get_list_items`: as mentioned in the previous comment, I'd like to
   see comments about subtle points like adding '/' or the recursive
   call to `_get_list_items()`.

 - `_get_list_items`: `status` isn't used and can be `_`.

 - `_get_list_items`: the cases where `values` is None don't seem to be
   tested.

 '''config_data_test.py'''

 I suggest adding a short comment about the intent of each test case.
 After looking at them closely I think I understand them, but some
 comments would have helped that pretty much.

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