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):

 I have some more points, but considering the amount of time we've
 spent for this ticket and the importance of these points, my last
 suggestion is the following two:

 - add some clarification comment to the following part of complete():
 {{{#!python
                 elif self._cmd_has_identifier_param(cmd):
                     # For tab-completion of identifiers, replace hardcoded
                     # hints with hints derived from the config data
                     hints = self._get_identifier_startswith(text)
 }}}
 (see the long discussion below)

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

 > added

 Which one?  I cannot easily identify it.

 With addressing these please merge and close.

 Now the discussion part:

 '''complete()'''

 Let me check my understanding.  In the original version:
 {{{#!python
                     hints = self._get_param_startswith(cmd.module,
 cmd.command,
                                                        text)
                     if cmd.module == CONFIG_MODULE_NAME:
                         # grm text has been stripped of slashes...
                         my_text = self.location + "/" +
 cur_line.rpartition(" ")[2]
                         list =
 self.config_data.get_config_item_list(my_text.rpartition("/")[0], True)
                         hints.extend([val for val in list if
 val.startswith(my_text[1:])])
                         # remove the common prefix from the hints so we
 don't get it twice
                         hints = self.remove_prefix(hints,
 my_text.rpartition("/")[0])
 }}}

 `_get_param_startswith()` collects all possible command parameters,
 regardless of whether it's a configuration related command.  If it's a
 configuration related command, the returned hints contain special
 parameter names like IDENTIFIER_PARAM itself, or "value" or
 "argument".  While keeping them in hints, this code then checks the
 module name, and if it's for configuration get_config_item_list()
 collects configuration item identifiers and add them to the hints.
 This is why we had the very original problem of this ticket, having
 "identifier" and "argument" in the completion list.

 This branch fixes the problem as follows:
 {{{#!python
                 elif self._cmd_has_identifier_param(cmd):
                     # For tab-completion of identifiers, replace hardcoded
                     # hints with hints derived from the config data
                     hints = self._get_identifier_startswith(text)
                 else:
                     hints = self._get_param_startswith(cmd.module,
 cmd.command,
                                                        text)
 }}}

 In this version, instead of unconditionally builds command parameter
 names, it first checks if the command has a special name of parameter
 IDENTIFIER_PARAM.  And, if it does, it only collects config item
 identifiers, ignoring all command parameter names.  In effect, this
 does this check in the original version:
 {{{#!python
                     if cmd.module == CONFIG_MODULE_NAME:
 }}}
 before doing `_get_param_startswith()`.  Is my understanding correct?

 If my understanding so far is correct, I now see what was wrong with
 the original version and the revised version would fix that particular
 issue.  But I think we still need to discuss a few points.

 - If the command has the special parameter named IDENTIFIER_PARAM,
   the new version now ignores all other command parameters that the
   command possibly has.  In fact, the new version now cannot suggest
   "help" after "config show".
 - some part of the original code logic now seems to be missing:
 {{{#!python
                         my_text = self.location + "/" +
 cur_line.rpartition(" ")[2]
 ...
                         # remove the common prefix from the hints so we
 don't get it twice
                         hints = self.remove_prefix(hints,
 my_text.rpartition("/")[0])
 }}}
   According to your previous response comment, I at least understand
   that the first line is somehow related to "config go", and removing
   it would break that scenario (but that we'd rather defer it as
   "config go" itself is quite broken).  That's fine to me (I don't
   want to do any more stuff in this ticket especially if it's already
   broken), but what about remove_prefix()?
 - A minor point, the comment in the new version ("For
   tab-completion...") doesn't seem to be really correct in that it
   actually doesn't "replace" anything (this is the first creation of
   hints)

 To address the first point more comprehensively, I guess we need some
 concept of "command parameter type" (which might just be recognized
 via the parameter name like IDENTIFIER_PARAM), and introduce a
 mechanism of how to expand various types of parameters (including just
 ignoring some).  On top of that, we always expand all parameters of
 the command.

 Also, not related to this branch, but another issue with complete() in
 terms of understandability is that it mixes if-else and try-except to
 implement a single set of conditions:

 {{{#!python
         line; if no module is given yet, it will list all modules. If a
         module is given, but no command, it will complete with module
         commands. If both have been given, it will create the hints based
 on
         the command parameters.
 }}}

 (btw this documentation helps, thanks).  The "if no module is given"
 part is caught in an `except`:
 {{{#!python
             except CmdModuleNameFormatError:
                 if not text:
                     hints = self.get_module_names()
 }}}
 (right?) while the rest is generally handled in the if-else logic
 above this.  It would be very difficult for a first-time reader to
 understand the whole logic flow of this method without the detailed
 documentation.  The documentation helps, but it then reveals that the
 implementation is really ugly.

 That said, I don't think it's productive to spend even more time for
 addressing these issues case by case basis.  My suggestion for this
 ticket is: just adding some comments about the restriction here:
 {{{#!python
                 elif self._cmd_has_identifier_param(cmd):
                     # For tab-completion of identifiers, replace hardcoded
                     # hints with hints derived from the config data
                     hints = self._get_identifier_startswith(text)
 }}}
 (that "config go" may not work, "help" won't appear in some cases, and
 if the missing remove_prefix() has some adversary effect, mention that
 too), and close the ticket hoping we'll address these in a cleaner way
 in the next generation.

 Some specific suggestions:

 - revise the comment
 - maybe rename IDENTIFIER_PARAM to something like CFGITEM_IDENTIFIER_PARAM


 '''config_data.py'''

 - `_get_list_items` docstring has a duplicate:
 {{{#!python
         """This method is used in get_config_item_list, to add list
 ...
            _get_list_items("Module/list")
                where the list contains 2 elements, returns
                [ "Module/list[0]", "Module/list[1]" ]
            _get_list_items("Module/list")
                where the list contains 2 elements, returns
                [ "Module/list[0]", "Module/list[1]" ]
         """
 }}}

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