Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2254 closed defect (fixed)

bindctl tab completion confusing output

Reported by: jreed Owned by: jelte
Priority: medium Milestone: Sprint-20121009
Component: ~bind-ctl (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 14 Internal?: no

Description

The following shows when I press tab twice:

> config show 
Boss/components       Logging/loggers       identifier
Cmdctl/accounts_file  argument              tsig_keys/keys
Cmdctl/cert_file      data_sources/classes  
Cmdctl/key_file       help                  
> config show argument
> 

It has "identifier" and "argument" which look confusing.

It should also complain about non-existing command. I thought there was a ticket for that, but cannot find now.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by jelte

FYI, this is still a remnant of the original way it worked, where all commands where of the form

Foo identifier="doStuff" argument="My arguments"

I also wanted to fix another tab-completion thing; it breaks through lists (i.e. after foo/bar[3]/ tab completion does not work anymore). Perhaps we can include that in this ticket as well

comment:2 Changed 7 years ago by jelte

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120925

comment:4 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

The actual change as proposed in this ticket is quite small (first commit, unfortunately that method is rather hard to test in its current form, we should really pull out the logic from the readline/interaction stuff there at some point)

Then I fixed two more things; as someone mentioned on jabber, 'config remove Boss' crashed bindctl, it now gives an error, and the biggest change was to fix tab-completion for indexes).

comment:6 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:7 in reply to: ↑ 5 Changed 7 years ago by jreed

Replying to jelte:

Then I fixed two more things; as someone mentioned on jabber, 'config remove Boss' crashed bindctl, it now gives an error, and the biggest change was to fix tab-completion for indexes).

This should fix #2289 and #2290

comment:8 follow-up: Changed 7 years ago by jinmei

general

Don't we need a changelog for this fix?

first commit (1ed45f9349d76439f61d9ab88c0ce1ea84cc6711)

Where do exactly these two keywords come from? If they are a
"remnant", isn't it better to just clean them up where they are
generated?

If we still need to filter them out explicitly for some reason, I'd
like to avoid hardcoding specific keywords in this code. I'd define
the special keywords in a unified space and let the filter code refer
to it.

And, one minor thing:

            hints = [ h for h in hints if h not in [ 'argument', 'identifier']]

spacing policy on 'after [' and 'before ]' isn't consistent. I'd
change it to, e.g.:

            hints = [h for h in hints if h not in ['argument', 'identifier']]

or add a space after '[', etc.

list completion fix (483011e9e126f02863fc10c69278f55f204b5f17)

This chunk doesn't seem to be related to "list completion":

-                        hints = self.remove_prefix(hints, my_text.rpartition("/")[0])
+                        prefix, _, rest = my_text.rpartition("/")
+                        hints = self.remove_prefix(hints, prefix)
+                        # And prevent 'double addition' by also removing final
+                        # part matches
+                        hints = [ h for h in hints if h != rest ]

and while I guess what it tries to do, I cannot be 100% sure exactly
what kind of thing it tries to eliminate (and tests don't seem to
catch it). Can you give an example? Is this one of the cases you
couldn't write tests?

  • _get_list_items: overall, I cannot be confident about what this method tries to do and (partly as an obvious consequence of it) the revised code does what it's intended to do. If it's not only me, that may be partly because there are some missing test cases (see below), and maybe partly because of the known complexity of the config framework and its implementation. Overall, it's difficult to understand this code fragment without knowing many details of the framework. If it's only me, that's fine, but if not it indicates the current framework is quite difficult to maintain, and we'll need to apply the lesson to the config-ng.
  • in _get_list_items, several cases don't seem to be covered in tests:
                if spec_part['named_set_item_spec']['item_type'] == 'map' or\
                   spec_part['named_set_item_spec']['item_type'] == 'named_set':
                    subslash = "/" <= not covered
               ...
                if len(values) > 0:
               ...
                else:
                    return [ item_name ] <= not covered
    ...
                if len(values) > 0:
    ...
                else:
                    return [ item_name ] <= not covered
    
  • minor point: in _get_list_items, subslash is used only when len(values) > 0 so the construction could be deferred.
  • is it correct to add '/' in the case of item_type is map?
                if spec_part['named_set_item_spec']['item_type'] == 'map' or\
                   spec_part['named_set_item_spec']['item_type'] == 'named_set':
                    subslash = "/"
                values, status = self.get_value(item_name)
                if len(values) > 0:
                    return [ item_name + "/" + v + subslash for v in values.keys() ]
    
    (this is one of the points that confused me in the larger picture).
  • what's the point of the recursive call to _get_list_items?
                        result.extend(self._get_list_items(name))
    
    why is it only done for the list case? how deep could the recursion be (or does that matter?). (this is another point that confused me in the larger picture).
  • maybe unrelated to this branch, but when I type in TAB in the following:
    > config show data_sources/classes/IN[0]/cache-zones[
    
    I saw this:
    > config show data_sources/classes/IN[0]/cache-zones[cache-zones[
    
    which is annoying.
  • this change doesn't seem to be covered in tests,
    -        else:
    +        # ignore any
    +        elif not spec_part_is_any(spec):
    
    and partly because of that, I don't fully understand what's wrong with the original code. (This is also related to the big picture confusion).
  • about the tests: It's difficult to me to understand how exactly we see this change and what tests try to confirm, without understanding deeply the underlying config mechanism. Specifically, it's not obvious to me what get_config_item_list() is expected to return, probably without understanding how module_spec_from_file() and/or set_specification(). Can you provide some background information here? Is it possible to provide more direct tests for the exact changes?

ccsession.py and test

  • It's not clear to me how this solves the 'config remove Boss' problem:
    -        type_any = module_spec['item_type'] == 'any'
    +        type_any = isc.config.config_data.spec_part_is_any(module_spec)
    
  • the new test in ccsession_test seems to be related to this, but it's not clear to me what it tries to test. Please explain (maybe as comments in the test code).

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:10 in reply to: ↑ 8 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to jinmei:

general

Don't we need a changelog for this fix?

Yes

[bug]
Several bugs are fixed in bindctl; tab-completion now works within lists, the problem where sometimes the completion added a part twice has been solved, and it no longer suggests the confusing value 'argument' as a completion-hint. Additionally, bindctl no longer crashes upon input like 'config remove Boss'.

Where do exactly these two keywords come from? If they are a
"remnant", isn't it better to just clean them up where they are
generated?

They come from bindctl command interpreter parser code, and the way it defines what commands can have what arguments (for config the string 'argument' is in fact a valid argument). But I have now taken a slightly more intrusive approach, which also solves a few other things you mentioned, and one that I found while inspecting this;

  • first I refactored out the hints-for-identifier-collection (so it can be tested)
  • it is only used now if a command actually has an identifier as a possible parameter (so config diff and other no longer suggest identifiers to complete);
  • I also removed every word-boundary character in readline except whitespace; this was the real cause of the 'duplicate-addition' (something you also encountered below and which the hack was solving only partly, and it is now no longer necessary to remove the common prefixes, so the actual hints collection suddenly got a lot easier on the bindcmd side)

list completion fix (483011e9e126f02863fc10c69278f55f204b5f17)

This chunk doesn't seem to be related to "list completion":

-                        hints = self.remove_prefix(hints, my_text.rpartition("/")[0])
+                        prefix, _, rest = my_text.rpartition("/")
+                        hints = self.remove_prefix(hints, prefix)
+                        # And prevent 'double addition' by also removing final
+                        # part matches
+                        hints = [ h for h in hints if h != rest ]

and while I guess what it tries to do, I cannot be 100% sure exactly
what kind of thing it tries to eliminate (and tests don't seem to
catch it). Can you give an example? Is this one of the cases you
couldn't write tests?

this has now also been removed as it was no longer necessary, as the real cause was that '/' was considered a word boundary.

  • _get_list_items: overall, I cannot be confident about what this method tries to do and (partly as an obvious consequence of it) the revised code does what it's intended to do. If it's not only me, that may be partly because there are some missing test cases (see below), and maybe partly because of the known complexity of the config framework and its implementation. Overall, it's difficult to understand this code fragment without knowing many details of the framework. If it's only me, that's fine, but if not it indicates the current framework is quite difficult to maintain, and we'll need to apply the lesson to the config-ng.

Yes.

From a high-level point of view, the problem is that we have raw data here (not nicely wrapped in ducktyped classes), for which we need to do different things depending on both the type and the content of that data. To make it worse, we have 2 different sets of these (the specification and the actual current values) from which we both need data, again based on their actual contents.

This is why I have proposed we actually put the data in a wrapper class, one which also points to the specification that validated that data; If the classes themselves are a bit smarter, they can enumerate their own (possible) values, and default values, etc. So the bindctl side would just need to ask it to enumerate itself instead of all this highly scary stuff. But that needs much more work than we can hope to do before thursday.

In this specific case, this methods is there because for lists (and named sets) we cannot use the specification; we need to use the actual values that are currently set. This method tries to enumerate those.

  • in _get_list_items, several cases don't seem to be covered in tests:
                if spec_part['named_set_item_spec']['item_type'] == 'map' or\
                   spec_part['named_set_item_spec']['item_type'] == 'named_set':
                    subslash = "/" <= not covered
               ...
                if len(values) > 0:
               ...
                else:
                    return [ item_name ] <= not covered
    ...
                if len(values) > 0:
    ...
                else:
                    return [ item_name ] <= not covered
    

added

  • minor point: in _get_list_items, subslash is used only when len(values) > 0 so the construction could be deferred.

moved

  • is it correct to add '/' in the case of item_type is map? (this is one of the points that confused me in the larger picture).
  • what's the point of the recursive call to _get_list_items? why is it only done for the list case? how deep could the recursion be (or does that matter?). (this is another point that confused me in the larger picture).

both of these are rather arbitrary; we don't want to always recurse (since you'd get way too many possible completions), but for lists we do. The / is added in the non-recursion case so as a user you only have to press tab again to get the next set.

As the options and possible values grow, this needs to get smarter as well; I'm thinking that it should do like 2 or 3 levels, and it should page results if there are too many (or maybe it should try to reduce by lowering the level of recursion, then page if it's still too much).

  • maybe unrelated to this branch, but when I type in TAB in the following:
    > config show data_sources/classes/IN[0]/cache-zones[
    
    I saw this:
    > config show data_sources/classes/IN[0]/cache-zones[cache-zones[
    
    which is annoying.

see first part, this should now be fixed

  • this change doesn't seem to be covered in tests,
    -        else:
    +        # ignore any
    +        elif not spec_part_is_any(spec):
    
    and partly because of that, I don't fully understand what's wrong with the original code. (This is also related to the big picture confusion).
  • about the tests: It's difficult to me to understand how exactly we see this change and what tests try to confirm, without understanding deeply the underlying config mechanism. Specifically, it's not obvious to me what get_config_item_list() is expected to return, probably without understanding how module_spec_from_file() and/or set_specification(). Can you provide some background information here? Is it possible to provide more direct tests for the exact changes?

does the above explanation help? (module_spec_from_file() is a rather basic function that takes a file and parses the json in in, then checks the format, and set_specification() is used to put a result from that into the object that holds multiple of those.

ccsession.py and test

  • It's not clear to me how this solves the 'config remove Boss' problem:
    -        type_any = module_spec['item_type'] == 'any'
    +        type_any = isc.config.config_data.spec_part_is_any(module_spec)
    

the helper function does an extra type test (at this point the module_spec value may not even be a dict).

  • the new test in ccsession_test seems to be related to this, but it's not clear to me what it tries to test. Please explain (maybe as comments in the test code).

Done, I also found a second instance of this problem, made the same change (yep; another hint for more substansive refactors)

I also realized the exception type was wrong, it should be DataTypeError?, not DataNotFoundError?, changed as well.

comment:11 Changed 7 years ago 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:
        # 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)
    -    # This is a fix for the problem described in
    -    # http://bind10.isc.org/ticket/1345
    ...
    
  • _get_identifier_startswith: I suggest this for better readability:
            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:
    -                    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:
                    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:
                            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?
    -        else:
    +        # ignore any
    +        elif not spec_part_is_any(spec):
    
  • _get_list_items: to help understand it, I'd like to see docstring that explains the type of item_name and its expected form.
        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.

comment:12 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:13 Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Replying to 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:
        # 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)
    -    # 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:
            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:
    -                    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:
                    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:
                            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?
    -        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)

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

comment:14 follow-up: Changed 7 years ago 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():
                    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:

                    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:

                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:

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

        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:

            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:

                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:
            """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]" ]
            """
    

comment:15 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:16 in reply to: ↑ 14 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to 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():
                    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:
                            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:

        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:

            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:

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

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 14
Note: See TracTickets for help on using tickets.