BIND 10 #2254: bindctl tab completion confusing output
BIND 10 Development
do-not-reply at isc.org
Tue Sep 25 04:35:17 UTC 2012
#2254: bindctl tab completion confusing output
-------------------------------------+-------------------------------------
Reporter: jreed | Owner: jinmei
Type: | Status: reviewing
defect | Milestone:
Priority: | Sprint-20120925
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):
'''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:
{{{#!python
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.:
{{{#!python
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":
{{{#!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?
- `_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:
{{{#!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
}}}
-
- 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?
{{{#!python
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`?
{{{#!python
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,
{{{#!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?
'''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 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).
--
Ticket URL: <http://bind10.isc.org/ticket/2254#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list