Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

arrays with natural sorting #2698

Open
markus2330 opened this issue May 13, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@markus2330
Copy link
Contributor

commented May 13, 2019

As discussed in #182 and #2388 we could avoid the _ for array numbers if we internally escape #<nr> to #_..<nr> (elektraUnescapeKeyName):

As already discussed shortly, we could also extend the escaping logic for key names and avoid the _ prefixes. So it would be #10, and the (internally) unescaped version (see keyUnescapedName) would be #_10 for correct sorting.

Then the sorting would be still correct and the user would not be bothered by ___ (as mentioned recently in #2685)

Furthermore, we maybe should change the character for arrays to not be # as # is a shell comment (see #2388).

@markus2330 markus2330 added this to the 0.9.0 milestone May 13, 2019

@markus2330 markus2330 referenced this issue May 13, 2019

Open

error: warning array concept added #2685

1 of 9 tasks complete
@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

elektraEscapeKeyNamePart

Not sure that is the right place to add this. It is only called in keySetBaseName and keyAddBaseName as far as I can tell. Shouldn't it be done in elektraFinalizeName (or one of the unescape functions called from there)?

as # is a shell comment

It is also an INI comment which always makes for confusing code highlighting.


Also, if someone implements this: I think elektraUnescapeKeyName can be optimized by adding something like:

--- a/src/libs/elektra/internal.c
+++ b/src/libs/elektra/internal.c
@@ -715,6 +715,19 @@ size_t elektraUnescapeKeyName (const char * source, char * dest)
 
 	ELEKTRA_ASSERT (sp != NULL && dp != NULL, "Got null pointer sp: %p dp: %p", (void *) sp, (void *) dp);
 
+	if (strpbrk (sp, "\\%.") == NULL)
+	{
+		strcpy (dest, sp);
+		char * last = dp;
+		while ((dp = strchr (dp, '/')) != NULL)
+		{
+			*dp = '\0';
+			++dp;
+			last = dp;
+		}
+		return last - dest + strlen (last) + 1;
+	}
+
 	if (*sp == '/')
 	{
 		// handling for cascading names

I think this will improve performance, because it copies the string in one go and just skips to the chars that need replacing, if no real unescaping is needed.
Just by looking at how much code there is for the key name unescaping, I suspect more optimizations may be possible.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Thank you for looking into the details and giving the implementation hints! You are right, it would be
elektraUnescapeKeyName (or elektraUnescapeKeyNamePartBegin to check if it is an array and elektraUnescapeKeyNamePart to unescape the array).

It is also an INI comment which always makes for confusing code highlighting.

Thanks for the hint. So which character would be best?

@kodebach

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

So which character would be best?

Maybe ^. I can't think of a case where that would cause problems. In any case changing the character would be major change requiring huge effort.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Maybe ^. I can't think of a case where that would cause problems

Yes, ^ is quite nice. It may cause minor problems with "deadkeys" on some keyboards (that you need to press the button twice).

In any case changing the character would be major change requiring huge effort.

Of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.