BIND 10 #2254: bindctl tab completion confusing output

[BIND 10 Development]

#2254: bindctl tab completion confusing output
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jelte
                       Type:         |                Status:  closed
  defect                             |             Milestone:
                   Priority:         |  Sprint-20121009
  medium                             |            Resolution:  fixed
                  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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * status:  reviewing => closed
 * resolution:   => fixed


Comment:

 Replying to [comment:14 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)
 >

 updated

 > - `_get_list_items`: the cases where values is None don't seem to be
 >   tested.
 >
 > > added
 >
 > Which one?  I cannot easily identify it.
 >

 oh right it tested empty list, not none, sorry, added explicit test for
 that too


 > With addressing these please merge and close.
 >

 yay :)

 Done, merged, closing ticket.


 > Now the discussion part:
 >
 > '''complete()'''
 >

 <snip>

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

 Yes your understanding is correct.

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

 Yes. I thought about adding that explicitely but decided not to, at least
 for now.


 > - 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()?

 Right.

 The remove_prefix part was a workaround for the '/' being a word boundary
 in the original code, which is now no longer necessary (and was incomplete
 anyway, as we have found out)

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

 Right. However, to do it really right, I think we need to change the way
 commands are defined and parsed here in the first place; the original code
 was really written to use name=value pairs for all arguments, not
 positional parameters. A number of the problems we see here are still
 caused by that I think (for instance that it is not really possible, or at
 least very difficult, to see which parameter we would be expanding at this
 time. Hence the 'just use identifiers if one of the parameters is
 CFGITEM_IDENTIFIER_PARAM right now).

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

 ack.

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

 ok

 > Some specific suggestions:
 >
 > - revise the comment

 done

 > - maybe rename IDENTIFIER_PARAM to something like
 CFGITEM_IDENTIFIER_PARAM
 >

 made it CFG_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]" ]
 >         """
 > }}}

 oops, removed.

 As suggested, I am now merging and closing this ticket, you are welcome to
 respond further (either here, on-list, or through a new ticket).

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