keys that end with the separator show up as folder and <empty>
tessus opened this issue · 9 comments
Sorry that I've been opening a few issues over the past few days. But it means that someone is using your software. ;-)
When I create a key called test:
, it looks like this:
It has something to do with the way the namespaces are built. Maybe you have a quick fix, since you wrote the code in the first place.
How would you expect this to be shown?
The <empty>
is the fix. We could just show test:
but then what happens when you also add test:foo
. Should it show test:
as a key and test
as a folder with foo
in it?
Should it show
test:
as a key andtest
as a folder withfoo
in it?
Yes. Because test:
is a key and not a folder.
But test:foo
is a key as well and not a folder.
And since an empty string is a valid key in Redis as well, we need a way to handle empty keys. So using that same logic to handle empty folder keys kinda makes sense doesn't it?
I'm not really sure. When you put it like that, maybe. Empty keys are rather uncommon I would say. The only real empty key possible is set "" val
. In case of test:
there are 2 ways you can look at it:
- a key called
test:
- a namespace
test
with an empty key
I see it as the former, you apparently as the latter. I guess it is a matter of opinion. But maybe there's a way to create an option.
I'm open for suggestions. I don't have a strong feeling any ways. Maybe we should give the <empty>
a light grey color so it's clear it isn't the key name?
The way you would like it is really simple to fix:
diff --git a/index.php b/index.php
index fc271b6..f18feaa 100644
--- a/index.php
+++ b/index.php
@@ -34,6 +34,10 @@ if($redis) {
}
$key = explode($server['seperator'], $key);
+ if ($key[count($key) - 1] == '') {
+ array_pop($key);
+ $key[count($key) - 1] .= ':';
+ }
// $d will be a reference to the current namespace.
$d = &$namespaces;
I love options. On one side options need to be maintained, on the other side in most cases the benefits outweigh the effort.
It's your code, but I'd suggest an option like showEmptyNamespaceAsKey
or emptyNsAsKey
. If set, it will run above code. (Thanks, btw.)
The light grey color idea can be done in addition, so that there's a visual contrast to a real key.
As we have established earlier, there are 2 ways how a person can perceive such a situation. Both are perfectly valid, thus I'd allow people to choose the behavior to their liking.
P.S.: I can create a PR for such an option. We can always change stuff during the PR revirew/discussion....
I have just pushed this option and the grey color: c291de3
Let me know if you think it should be different.
Looks great. I left an inline comment for the commit. I will open a PR to address that
(set a default in includes/common.inc.php
.
I guess we don't need an additional database level option to override the global setting in this case.