Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

kdb-complete minor improvements #1310

Merged
merged 7 commits into from
Feb 8, 2017
+344 −128
Merged

kdb-complete minor improvements #1310

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9373eaa
kdb-complete: add support for namespace completion, don’t print leaf/…
e1528532 Jan 30, 2017
b10fd12
kdb-complete: update manpage
e1528532 Jan 30, 2017
efdd4ba
kdb-complete: fix documentation, fix unnecessary space in output
e1528532 Feb 1, 2017
5066db7
kdb-complete: add detection for missing / outdated namespaces
e1528532 Feb 3, 2017
4a55d43
kdb-complete: add bookmark support
e1528532 Feb 3, 2017
fc5cb51
kdb-complete: major refactoring and fixing all those small issues and…
e1528532 Feb 8, 2017
fdc3c1d
kdb-complete: reorder methods to have some structure
e1528532 Feb 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions doc/help/kdb-complete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ Where `path` is the path for which the user would like to receive completion sug
## DESCRIPTION

Show suggestions how the current name could be completed.
Suggestions will include existing key names, path segments of existing key names and mountpoints.
Additionally, the output will indicate wheter the given path is a node or a leaf in the hierarchy of keys.
Suggestions will include existing key names, path segments of existing key names, namespaces and mountpoints.
Additionally, the output will indicate wheter the given path is a node or a leaf in the hierarchy of keys,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:

…indicate wheter the…

Correction:

…indicate whether the…

nodes end with '/' as opposed to leaves.

## OPTIONS

Expand All @@ -26,7 +27,7 @@ Additionally, the output will indicate wheter the given path is a node or a leaf
- `-M`, `--max-depth`=<max-depth>:
Specify the maximum depth of completion suggestions (unlimited by default, 1 to show only the next level), inclusive.
- `-v`, `--verbose`:
Explain what is happening.
Give a more detailed output, showing the number of child nodes and the depth level.
- `-0`, `--null`:
Use binary 0 termination.
- `-d`, `--debug`:
Expand Down
79 changes: 55 additions & 24 deletions src/tools/kdb/complete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,11 @@ CompleteCommand::CompleteCommand ()
{
}

/*
* McCabe complexity of 11, 1 caused by verbose switch so actually its ok
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document what happens without arguments. Also fix SYNOPSIS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without arguments is like calling kdb complete "" (basically that means show me every possible completion if i haven't typed anything yet, which will be every possibility so namespaces, keys (expanded to nodes) and mountpoints). I'll document it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

int CompleteCommand::execute (const Cmdline & cl)
{
if (cl.arguments.size () != 1)
{
throw invalid_argument ("wrong number of arguments, 1 needed");
}
if (cl.maxDepth <= cl.minDepth)
{
throw invalid_argument ("the maximum depth has to be larger than the minimum depth");
Expand All @@ -49,10 +48,15 @@ int CompleteCommand::execute (const Cmdline & cl)
cout.unsetf (ios_base::skipws);
}

// Determine the actual root key, as for completion purpose originalRoot may not exist
// For namespace completion, we get all available keys via / as the actual root and filter via the argument
const bool hasArgument = cl.arguments.size () > 0;
const string originalInput = hasArgument ? cl.arguments[cl.arguments.size () - 1] : "";
const Key originalUnprocessedKey (originalInput, KEY_END);
KDB kdb;
const Key originalRoot = cl.createKey (cl.arguments.size () - 1);
Key root = originalRoot;
// Determine the actual root key, as for completion purpose originalRoot may not exist
// Maybe we want to complete an initial namespace, in that case bookmark expansion checks done by cl.createKey are useless
const Key originalRoot = originalUnprocessedKey.isValid () ? cl.createKey (cl.arguments.size () - 1) : originalUnprocessedKey;
Key root = originalUnprocessedKey.isValid () ? originalRoot : Key ("/", KEY_END);
KeySet ks;
printWarnings (cerr, root);

Expand All @@ -71,7 +75,7 @@ int CompleteCommand::execute (const Cmdline & cl)
// Now analyze the completion possibilities and print the results
addMountpoints (ks, root);
KeySet virtualKeys;
printResult (originalRoot, root, analyze (ks, root, virtualKeys, cl), virtualKeys, cl);
printResults (originalInput, originalRoot, root, analyze (ks, root, virtualKeys, cl), virtualKeys, cl);
printWarnings (cerr, root);

return 0;
Expand Down Expand Up @@ -102,6 +106,17 @@ void CompleteCommand::addMountpoints (KeySet & ks, const Key root)
printWarnings (cerr, mountpointPath);
}

void CompleteCommand::addNamespaces (map<Key, pair<int, int>> & hierarchy)
{
// Currently assumed to be fixed, and they are on level 0
const string namespaces[] = { "spec/", "proc/", "dir/", "user/", "system/" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great if we do not need to hardcode it or at least get a warning/error if we add an namespace. See keyGetNamespace.

To get all namespaces might be a good addition in the c++ binding (or at least in libtools).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to get all available namespaces, because i haven't found any? They only thing i've found are integer constants e.g. KEY_NS_DIR, but those can't really be translated back to a name like "dir/" right?

keyGetNamespace can be used to get a key's namespace yes, but what if there is nothing in e.g. proc yet, then even by iterating all keys and checking their namespace, we wouldn't be able to get all the available namespaces.

So if i add such method to the c++ bindings (though it might be even better to have such method on a lower level as its not really binding specific) wouldn't it end up returning a hardcoded list of namespaces?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will return a hard-coded list of namespaces. But the function should return a warning if we choose to extend the namespaces. It is explained in the docu of keyGetNamespace, simply do it like in the loop example there. https://doc.libelektra.org/api/latest/html/group__keyname.html#gafc3ca03ed10f87eb59bdc02cf2a0de8d

But you are right, it might be overkill to add such trivial code in the binding or libtools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i did something which would detect both outdated namespaces and missing namespaces, however, i'm not really happy with the check being executed each single time and probably ruining completions at some point (when a namespace change happens). I though about only displaying it when using -v or -d, but then it would probably take a while to notice that kdb complete is not up to date.

for (const auto ns : namespaces)
{
Key nsKey (ns, KEY_END);
hierarchy[nsKey] = pair<int, int> (1, 0);
}
}

/*
* McCabe complexity of 13, 3 caused by debug switches so actually its ok
*/
Expand All @@ -111,6 +126,7 @@ const map<Key, pair<int, int>> CompleteCommand::analyze (const KeySet & ks, cons
stack<Key> keyStack;
Key parent;
Key last;
addNamespaces (hierarchy);

ks.rewind ();
if (!(ks.next ()))
Expand Down Expand Up @@ -184,16 +200,17 @@ void CompleteCommand::increaseCount (map<Key, pair<int, int>> & hierarchy, const
}

/*
* McCabe complexity of 12, 2 caused by debug/verbose switches so actually its ok
* McCabe complexity of 11, 3 caused by verbose switch so actually its ok
*/
void CompleteCommand::printResult (const Key originalRoot, const Key root, const map<Key, pair<int, int>> & hierarchy,
const KeySet & virtualKeys, const Cmdline & cl)
void CompleteCommand::printResults (const string originalInput, const Key originalRoot, const Key root,
const map<Key, pair<int, int>> & hierarchy, const KeySet & virtualKeys, const Cmdline & cl)
{
const function<bool(string)> filterPredicate = determineFilterPredicate (originalRoot, root);
const function<bool(string)> filterPredicate = determineFilterPredicate (originalInput, originalRoot, root);

// Adjust the output offset, in case the given string exists in the hierarchy but not in the original ks
// or for namespace completion
const bool limitMaxDepth = cl.maxDepth != numeric_limits<int>::max ();
const int offset = originalRoot != root && virtualKeys.lookup (originalRoot);
const int offset = originalRoot != root && virtualKeys.lookup (originalRoot) ? 1 : (originalRoot.getFullName ().empty () ? -1 : 0);
const int minDepth = cl.minDepth + offset;
const int maxDepth = limitMaxDepth ? cl.maxDepth + offset : cl.maxDepth;
if (cl.verbose)
Expand All @@ -210,35 +227,49 @@ void CompleteCommand::printResult (const Key originalRoot, const Key root, const
cout << endl;
}

if (cl.debug)
for (const auto & it : hierarchy)
{
cout << endl << "Dumping whole analyzation results" << endl;
for (const auto & it : hierarchy)
if (filterPredicate (it.first.getFullName ()) && it.second.second > minDepth && it.second.second <= maxDepth)
{
cout << it.first << " " << it.second.first << " " << it.second.second << endl;
printResult (it, cl.verbose);
}
cout << endl;
}
}

for (const auto & it : hierarchy)
void CompleteCommand::printResult (const pair<Key, pair<int, int>> & current, const bool verbose)
{
cout << current.first;
if (current.second.first > 1)
{
if (filterPredicate (it.first.getFullName ()) && it.second.second > minDepth && it.second.second <= maxDepth)
cout << "/ ";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you add a trailing whitespace character after /? I would prefer no trailing whitespace, since otherwise the Fish completion commands have to pipe the output of kdb complete trough sed 's/ *$//' or something similar. Otherwise a completion like user/ - containing trailing whitespace – shows up as user/\ , which is not as helpful as the correct suggestion user/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, of course this makes not much sense, moved the additional whitespace to the verbose output where it separates the number of children from the keyname.

if (verbose)
{
cout << it.first << (it.second.first > 1 ? " node " + to_string (it.second.first - 1) : " leaf") << endl;
cout << "node " << to_string (current.second.first - 1);
}
}
else if (verbose)
{
cout << " leaf 0";
}
if (verbose)
{
cout << " " << current.second.second;
}
cout << endl;
}

const function<bool(string)> CompleteCommand::determineFilterPredicate (const Key originalRoot, const Key root)
const function<bool(string)> CompleteCommand::determineFilterPredicate (const string originalInput, const Key originalRoot, const Key root)
{
if (root == originalRoot)
{
return [](string) { return true; };
}
const string fullName = originalRoot.getFullName ();

const bool namespaceCompletion = originalRoot.getFullName ().empty ();
const string fullName = namespaceCompletion ? originalInput : originalRoot.getFullName ();
return [=](string test) {
// For cascading keys, we ignore the preceding namespace for filtering
const int cascadationOffset = (root.isCascading () ? test.find ("/") : 0);
const int cascadationOffset = (!namespaceCompletion && root.isCascading () ? test.find ("/") : 0);
return fullName.size () <= test.size () && equal (fullName.begin (), fullName.end (), test.begin () + cascadationOffset);
};
}
Expand Down
10 changes: 7 additions & 3 deletions src/tools/kdb/complete.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,18 @@ class CompleteCommand : public Command

private:
void addMountpoints (kdb::KeySet & ks, const kdb::Key root);
void addNamespaces (std::map<kdb::Key, std::pair<int, int>> & hierarchy);
const std::map<kdb::Key, std::pair<int, int>> analyze (const kdb::KeySet & ks, const kdb::Key root, kdb::KeySet & virtualKeys,
const Cmdline & cmdLine);
const kdb::Key getParentKey (const kdb::Key key);
void increaseCount (std::map<kdb::Key, std::pair<int, int>> & hierarchy, const kdb::Key key,
const std::function<int(int)> depthIncreaser);
void printResult (const kdb::Key originalRoot, const kdb::Key root, const std::map<kdb::Key, std::pair<int, int>> & hierarchy,
const kdb::KeySet & virtualKeys, const Cmdline & cmdLine);
const std::function<bool(std::string)> determineFilterPredicate (const kdb::Key originalRoot, const kdb::Key root);
void printResults (const std::string originalInput, const kdb::Key originalRoot, const kdb::Key root,
const std::map<kdb::Key, std::pair<int, int>> & hierarchy, const kdb::KeySet & virtualKeys,
const Cmdline & cmdLine);
void printResult (const std::pair<kdb::Key, std::pair<int, int>> & current, const bool verbose);
const std::function<bool(std::string)> determineFilterPredicate (const std::string originalInput, const kdb::Key originalRoot,
const kdb::Key root);
};

#endif