BIND 10 #2254: bindctl tab completion confusing output
BIND 10 Development
do-not-reply at isc.org
Tue Sep 25 23:16:46 UTC 2012
#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:8 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":
> {{{#!diff
> - 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:
> {{{#!python
> 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,
> {{{#!diff
> - 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:
> {{{#!python
> - 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.
--
Ticket URL: <http://bind10.isc.org/ticket/2254#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list