kdb-complete minor improvements #1310

e1528532 · 2017-01-30T19:13:03Z

Purpose

This PR is intended to add smaller improvements for the kdb complete tool as requested in #1296.

Checklist

commit messages are fine (with references to issues)
I ran all tests and everything went fine
I added unit tests
affected documentation is fixed
I added code comments, logging, and assertions
meta data is updated (e.g. README.md of plugins)

@sanssecours @reox Is it more usable for your use cases now?

…node info without verbose parameter

markus2330

Thank you for the fix!

Lets give other people also a chance to review it.

@sanssecours are you interested?

markus2330 · 2017-01-30T19:53:32Z

src/tools/kdb/complete.cpp

 int CompleteCommand::execute (const Cmdline & cl)
 {
-	if (cl.arguments.size () != 1)


Please document what happens without arguments. Also fix SYNOPSIS

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

markus2330 · 2017-01-30T19:54:36Z

src/tools/kdb/complete.cpp

+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/" };


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

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?

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.

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.

sanssecours

I really like the improvements. Thank you.

sanssecours · 2017-01-30T22:16:29Z

src/tools/kdb/complete.cpp

 	{
-		if (filterPredicate (it.first.getFullName ()) && it.second.second > minDepth && it.second.second <= maxDepth)
+		cout << "/ ";


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

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.

sanssecours · 2017-01-30T22:23:14Z

doc/help/kdb-complete.md

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


Typo:

…indicate wheter the…

Correction:

…indicate whether the…

sanssecours · 2017-01-30T23:04:14Z

@sanssecours Is it more usable for your use cases now?

Yes, looks great! I have another feature request though. Would it be possible, for kdb complete to only suggests the namespaces

dir
proc
spec and
user

if they exist currently?

e1528532 · 2017-01-30T23:26:38Z

if they exist currently == if they are a node (contain entries)?

Well you sort of already have this behavior, if there is nothing in user, it will be a leaf and displayed as 'user', if there is anything in it, its 'user/' due to our node/leaf naming convention. Same applies to the other namespaces. Can you work with that or do you think it makes sense to always filter the empty namespaces away from kdb complete? If we decide to filter, there would be no way to get completions to "user" when typing "us", given there is currently nothing in user. Unless we'd make another option for that.

sanssecours · 2017-01-31T08:34:01Z

if they exist currently == if they are a node (contain entries)?

Yes.

Can you work with that or do you think it makes sense to always filter the empty namespaces away from kdb complete?

I can certainly can work with it 😊. However, while I think there are some situations, where it makes sense to suggest an empty namespace such as user (e.g. kdb set), I would prefer they just do not show up at all. This would also go well with the directory-like structure of namespaces. For example, I would not expect that a shell would suggest a non-existent directory if I press ⇥ after I entered the command ls. Just like that, I would not expect kdb ls to show the entries:

dir
proc
spec

if they currently do not exist.

If we decide to filter, there would be no way to get completions to "user" when typing "us", given there is currently nothing in user.

You are right. In this case kdb complete should just print an empty list in my opinion, since no key starting with us exists currently.

markus2330 · 2017-01-31T13:02:00Z

You are right. In this case kdb complete should just print an empty list in my opinion, since no key starting with us exists currently.

I think the completion of user (even if no such key exists) plays well with the completion to mountpoints.

Also the behaviour that you either get user or user/ speaks for leaving it as-is.

And keep in mind that in Elektra only keys exist, sections/directories can be invented in top of it, but are not an essential part of it. So the slash at end actually says if there are keys below it, not if something is a "directory" or "section".

Is there any usability issue for completing even if a key does not exist? For example, in zsh ls ~ autocompletes to every user, even if such a user does not have a homedirectory:

> ls ~no<tab>
> ls ~nobody
ls: cannot access /nonexistent: No such file or directory

I never had any troubles with this behaviour and would even expect to see all users.

Btw. bookmark completion is currently missing. (Those keys starting with +, see man kdb).

sanssecours · 2017-01-31T14:24:45Z

Is there any usability issue for completing even if a key does not exist?

For me the answer to this question is yes: I do not like useless suggestions :o). I guess the Fish completion code should just strip leaves if they do not make sense in the current context. Although this only makes sense for the first level, since only in this case kdb complete shows non-existent entries.

In conclusion: The current behaviour works for me too, I am not very happy with it though.

markus2330 · 2017-01-31T14:36:04Z

Hard to say which behaviour is best without trying it. I didn't have any issues with "useless" suggestions so far. On contrary, I switched from bash to zsh to get more suggestions.

But I would be in favour that all completions (zsh, bash, fish) complete in the same way (unless there are really differences in how particular shell users expect something to complete, is this the case?), so I would prefer if you do not make workarounds within the fish completion (also takes a lot of time to do that), but we make it configurable within kdb complete (if it is really impossible to come to an agreement which everyone finds okay). Then one can simply use kdb set +kdb/myfancycompletion and get a different completion-style, independent of the shell.

Seems like there is no way around discussing it in the meeting after all ;)

e1528532 · 2017-02-03T11:08:00Z

added bookmark support, it currently will show suggestions like following:
assume you have a bookmark +bm which points to user/test, and there is a key user/test/key:

kdb complete +bm
user/test/key

or should it look like

kdb complete +bm
+bm/key

And as discussed in the meeting, i'll leave namespace completion like it currently is (always suggestion the namespaces, even if they are empty). If the bookmark handling is ok that way, i think this is pretty much finished and can be merged.

markus2330 · 2017-02-03T12:29:28Z

zsh is inconsistent in such completions:

> echo =kdb<tab>
kdb         kdb-full    kdb-static
> echo =kdb-full<tab>
> echo /usr/bin/kdb-full

So in this case it only completes to a full name, once a "bookmark" (or command alias in this case) was found.

The ! completion directly and always substitutes to the full command.

But on path substitution, it keeps the "bookmark":

echo ~elektra/<tab>
files which are in folder ~elektra
echo ~elektra/libelektra/<tab>
files which are in folder ~elektra/libelektra

Thus you already implemented it in the first way, I would say let us keep it that way.

Btw. what happens when one does kdb complete +b and there exist multiple bookmarks that start with b?

markus2330 · 2017-02-03T12:30:54Z

Btw. maybe make a quick recheck if the tool never fails with an error. As @sanssecours pointed out, the shell completions expect to always get an answer.

markus2330 · 2017-02-06T17:02:30Z

@e1528532 Please finish the PR so that we can merge #1325.

… inconsistencies avoid output in normal mode to avoid destroying completions

e1528532 · 2017-02-08T12:47:56Z

So finally it should be more or less done. As i found quite a number of small inconsistencies and minor issues with the last version, and fixing them turned out to make the code worse and worse, i decided its necessary to refactor it a bit. It quite grew in terms of requirements, supporting namespaces, mountpoints, bookmarks and initially i haven't thought about all those cases.

Now it should be easier to understand the way it works (imo) and also to possibly extend it even further in the future.
Bookmarks are fully supported now, bookmark completion was missing and has also been added (suppose bookmarks +testbm1 and +testbm2, writing kdb complete +testbm now would suggest you both, writing kdb complete +testbm1 will resolve it and show the completion for the bookmark's destination like it was before). Bookmarks to namespaces or incomplete paths will also work (whyever you wanna do that, but those would have failed before and after the refactoring it was very easy to handle).
Also the use of min/max depth was a bit buggy and inconsistent before, i've streamlined that.
I've changed most of the output and warnings to only show in debug/verbose mode. As completions will mostly use this tool, encountering things like newly added namespaces should not lead to a warning i think as its not really critical but would likely break the completion, which would still work fine except not suggesting the new namespace.

So I've tested quite a lot of different cases and the tool never failed now, in the worst case you get no suggestions. And i've also installed a linux to run valgrind on it, which reports 0 errors/leaks.

markus2330 · 2017-02-08T15:31:42Z

Thank you, great work!

@reox bash and zsh completion cannot await to get these new features ;)

e1528532 added 2 commits January 30, 2017 19:47

kdb-complete: add support for namespace completion, don’t print leaf/…

9373eaa

…node info without verbose parameter

kdb-complete: update manpage

b10fd12

e1528532 added enhancement work in progress labels Jan 30, 2017

This was referenced Jan 30, 2017

KDB Complete: Minor Issues #1296

Closed

project diary #1087

Closed

markus2330 suggested changes Jan 30, 2017

View reviewed changes

sanssecours suggested changes Jan 30, 2017

View reviewed changes

kdb-complete: fix documentation, fix unnecessary space in output

efdd4ba

sanssecours approved these changes Feb 1, 2017

View reviewed changes

e1528532 added 2 commits February 3, 2017 10:51

kdb-complete: add detection for missing / outdated namespaces

5066db7

kdb-complete: add bookmark support

4a55d43

sanssecours mentioned this pull request Feb 6, 2017

Add KDB Completions for Fish #1325

Merged

2 tasks

markus2330 approved these changes Feb 6, 2017

View reviewed changes

e1528532 added 2 commits February 8, 2017 13:34

kdb-complete: major refactoring and fixing all those small issues and…

fc5cb51

… inconsistencies avoid output in normal mode to avoid destroying completions

kdb-complete: reorder methods to have some structure

fdc3c1d

e1528532 added ready to merge and removed work in progress labels Feb 8, 2017

markus2330 merged commit 3fb7c94 into ElektraInitiative:master Feb 8, 2017

rkamisetti792 mentioned this pull request Oct 6, 2022

[Snyk] Security upgrade node-fetch from 1.7.3 to 2.6.7 rkamisetti792/libelektra#61

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kdb-complete minor improvements #1310

kdb-complete minor improvements #1310

e1528532 commented Jan 30, 2017

markus2330 left a comment

markus2330 Jan 30, 2017

e1528532 Jan 31, 2017

e1528532 Feb 3, 2017

markus2330 Jan 30, 2017

e1528532 Jan 31, 2017

markus2330 Jan 31, 2017

e1528532 Feb 3, 2017

sanssecours left a comment

sanssecours Jan 30, 2017

e1528532 Jan 31, 2017

sanssecours Jan 30, 2017

sanssecours commented Jan 30, 2017

e1528532 commented Jan 30, 2017

sanssecours commented Jan 31, 2017

markus2330 commented Jan 31, 2017 •

edited

Loading

sanssecours commented Jan 31, 2017

markus2330 commented Jan 31, 2017

e1528532 commented Feb 3, 2017

markus2330 commented Feb 3, 2017

markus2330 commented Feb 3, 2017

markus2330 commented Feb 6, 2017

e1528532 commented Feb 8, 2017 •

edited

Loading

markus2330 commented Feb 8, 2017

kdb-complete minor improvements #1310

kdb-complete minor improvements #1310

Conversation

e1528532 commented Jan 30, 2017

Purpose

Checklist

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanssecours left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanssecours commented Jan 30, 2017

e1528532 commented Jan 30, 2017

sanssecours commented Jan 31, 2017

markus2330 commented Jan 31, 2017 • edited Loading

sanssecours commented Jan 31, 2017

markus2330 commented Jan 31, 2017

e1528532 commented Feb 3, 2017

markus2330 commented Feb 3, 2017

markus2330 commented Feb 3, 2017

markus2330 commented Feb 6, 2017

e1528532 commented Feb 8, 2017 • edited Loading

markus2330 commented Feb 8, 2017

markus2330 commented Jan 31, 2017 •

edited

Loading

e1528532 commented Feb 8, 2017 •

edited

Loading