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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

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

 noted. And my apologies.

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

 I'll check both again once this has been approved and merged, and not
 before (so as not to make the piggyback issue any lengthier).

 > '''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?

 Newlines and tabs can be removed; however, spaces are certainly necessary
 (for distinguising between modules, commands, and separate arguments,
 otherwise it can only complete the module name).

 Reduced to only spaces.

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

 added a slightly revised text, with more explanation and both these
 tickets.

 > }}}
 > - `_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)

 done

 > - `_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).
 >

 added

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

 Previously, it assumed that if the module was 'config', the parameter of
 the command was always an identifier; so even with commands like 'config
 diff' it would offer completion of identifiers.

 With these changes, it actually checks the parameters it knows for the
 command that is being entered, and only does identifier-completion if the
 command has a parameter which looks like it is an identifier (currently
 decided by the magic string IDENTIFIER_PARAM, which is 'identifier'). In
 those cases it does not look into the command definition but does the
 identifier completion instead.

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

 ah, of course, done.

 > - I'd suggest adding doc for complete(), with the type and meaning of
 >   parameters and possible examples.

 added, initially only refered to readline and cmd but accidentally wrote
 quite a long description anyway :p

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

 It was there to work together with a previously self.location (which is
 set after 'config go' commands). The logic should go into the
 _get_identifier_startswith() However, there are also several unrelated
 problems with config go, and not just in tab-completion. Rather than fix
 them here, it's probably better to defer those as well (probably until
 after an overhaul). So for now I removed that line entirely (instead of
 replacing it with something more clear that in the end won't work either).

 > '''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`)

 ok. For consistency I renamed it everywhere in the test files (renamed the
 class, and all instances where cmd=BindCmdParser); also fixed rest of the
 long lines there. This is in a separate commit in case you care :)
 (9b730d026d634dbefdc50eea9d63df77201e30f4)

 > - minor point: the variable name 'tool' sound confusing to me.
 >

 I guess it stands for 'command line tool'. Left that as-is for now.

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

 It was a problem I encountered when going through the code checking the
 'config remove Boss' problem; technically it was another issue, but when
 showing items it resulted in trying to get the wrong config data and could
 potentially also crash or at least not show data that it should (a result
 of passing around primitive types here, and the old code made a wrong
 assumption without checking, yes this is another patch that should be
 fundamentally solved by starting to use real classes and duck typing)

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

 added

 > - `_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
 >
 >            """
 > }}}
 >

 done

 > - `_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()`.
 >

 done

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

 ack done

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

 added

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

 I've done so for new tests I'm adding, and existing cases I'm changing,
 but did not go through the entire test file. Did you have any specific
 ones in mind or do you want me to add descriptions to all of them? :)

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