Kdb cli rewrite #4922

hannes99 · 2023-05-11T09:54:50Z

Added tests and if a command does not exist yet in C, the C++ implementation will be used. And due to #4444 the new implementation does not rely on specload anymore, support for it (kdb --elektra-spec) could be added easily if that would still make sense.

Basics

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which contains _(my name)_)
Please always add them to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
- fix the CI itself (or rebase if already fixed)
The PR is rebased with current master.

Checklist

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation (see Documentation Guidelines)
I fixed all affected decisions (see Decision Process)
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in reuse syntax

Review

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if everything is done and no further pushes are planned by you.

src/tools/kdb/get.c

src/tools/kdb/cmerge.c

src/tools/kdb/colors.c

src/tools/kdb/mv.c

src/tools/kdb/cp.c

src/tools/kdb/rm.c

src/tools/kdb/get.c

src/tools/kdb/colors.c

markus2330

Unfortunately still no reviews. Do more reviews for others and specifically ask others to do a review.

Anyway, the PR is a big progress. I am okay with replacing individual commands but without the possibility to compile kdb completely without C++, the whole PR only goes the half way. This does not necessarily need to happen within this PR, though.

The main issue is that the release notes do not properly describe what was done. In particular it should point to tutorials/tests that fix bugs etc. What are your contributions? Which advantage to we have if we merge this PR?

markus2330 · 2023-05-18T05:49:26Z

doc/news/_preparation_next_release.md

@@ -324,6 +324,15 @@ This section keeps you up-to-date with the multi-language support provided by El
 - <<TODO>>
 - <<TODO>>

+### KDB


Please make highlight out of it.

markus2330 · 2023-05-18T05:50:15Z

doc/news/_preparation_next_release.md

@@ -324,6 +324,15 @@ This section keeps you up-to-date with the multi-language support provided by El
 - <<TODO>>
 - <<TODO>>

+### KDB
+
+- Replace C++ of the CLI by a C version, C++ is fallback for not yet implemented commands _(@hannes99)_


How is it possible to compile without having C++ (which was the goal)?

markus2330 · 2023-05-18T05:51:13Z

doc/news/_preparation_next_release.md

+- Disable cascading writes as described in #3742
+- Fix inconsistent return values #1563


Explain what exactly was done here and give examples (or link to new tests that show correct behavior).

markus2330 · 2023-05-18T05:52:39Z

doc/news/_preparation_next_release.md

+- Add extra tests for CLI commands
+- Disable cascading writes as described in #3742
+- Fix inconsistent return values #1563
+- Update documentation, tutorials and examples where they would not match the new C version


Link to the places where relevant/bigger changes were done.

markus2330 · 2023-05-18T05:55:34Z

doc/tutorials/cascading.md

@@ -91,43 +91,6 @@ The **proc** namespace is not accessible by the command line tool **kdb**, as it

 The **spec** namespace is used to store metadata about keys and therefore Elektra handles the **spec** namespace differently to other namespaces. The following part of the tutorial is dedicated to the impact of the **spec** namespace on cascading lookups.

-## Write Operations and the cascading Namespace
-
-If a value is to be written to a cascading key, i.e., a key starting with '/', only cascading keys that resolve to an existing key will be used.


Demonstrate that it fails.

markus2330 · 2023-05-18T05:56:27Z

src/tools/kdb/cmerge.c

@@ -0,0 +1,254 @@
+/**


We only want cmerge and it should be called merge, the old merge can be removed. See also #3131

tmakar

Very good work here! I added some notes. For some comments it makes sense to check other occurences.

tmakar · 2023-05-18T07:20:04Z

src/tools/kdb/basename.h

+
+#include <kdb.h>
+
+/**


This is very minor, but what kind of function documentation do we follow in general? For some I see that we are doing documentation in the *.c files and some things are documented in *.h.

tmakar · 2023-05-18T07:27:44Z

src/tools/kdb/get.c

+	if (toLookUp == NULL)
+	{
+		ELEKTRA_SET_VALIDATION_SEMANTIC_ERRORF (errorKey, "'%s' is not a valid key name", keyName (toLookUp));
+		keyDel (toLookUp);


What about moving all the free memory code to cleanup section and check there if the variable is NULL and if not free the memory?

tmakar · 2023-05-18T07:30:04Z

src/tools/kdb/get.h

+Key * printTrace (KeySet * ks, Key * key, Key * found, elektraLookupFlags options);
+Key * warnOnMeta (KeySet * ks, Key * key, Key * found, elektraLookupFlags options);
+void printOptions (elektraLookupFlags options);
+void setCallback (Key * key, Key * (*f) (KeySet * ks, Key * key, Key * found, elektraLookupFlags flags));


I would suggest describing what kind of callback should be set here. The signature of *f is not really intuitive. And maybe also add some description for what the callback is needed.

tmakar · 2023-05-18T07:32:41Z

src/tools/kdb/meta-get.c

+
+	const char * metaName = GET_OPTION (options, "metaname");
+
+	Key * toLookUp = keyNew (name, KEY_END);


I would suggest moving toLookUp key initialization right before Key * key = ksLookup (conf, toLookUp, KDB_O_NONE);. IIRC, I have read in our coding conventions CODING.md that we should initialize code as late as possible and it is good practice in general.

tmakar · 2023-05-18T07:34:49Z

src/tools/kdb/regex-wrapper.h

Maybe some description in this header file 🤔 ?

... and remove cpp implementation

.. remove cpp implementation and fix #3131

... and (partial) fix #3742, fix #4028, fix #1164

... and (partial) fix #3742

.. and fix #3648

... and fix #3742

hannes99 requested review from atmaxinger, markus2330 and tmakar May 11, 2023 09:54

hannes99 force-pushed the kdb_cli_rewrite branch from 13fcd7d to 0c7a71e Compare May 11, 2023 10:35

hannes99 added the work in progress label May 11, 2023

github-advanced-security bot found potential problems May 11, 2023

View reviewed changes

hannes99 force-pushed the kdb_cli_rewrite branch 12 times, most recently from 35e9cbc to adb7dfe Compare May 12, 2023 13:06

github-advanced-security bot found potential problems May 12, 2023

View reviewed changes

src/tools/kdb/colors.c Fixed Show fixed Hide fixed

hannes99 force-pushed the kdb_cli_rewrite branch 5 times, most recently from 9cf997c to 7458acd Compare May 14, 2023 08:25

github-advanced-security bot found potential problems May 14, 2023

View reviewed changes

src/tools/kdb/colors.c Fixed Show fixed Hide fixed

hannes99 force-pushed the kdb_cli_rewrite branch 5 times, most recently from 32fa246 to d79b382 Compare May 14, 2023 12:34

markus2330 suggested changes May 18, 2023

View reviewed changes

hannes99 force-pushed the kdb_cli_rewrite branch from dcae111 to 5fe0c43 Compare May 18, 2023 06:34

tmakar suggested changes May 18, 2023

View reviewed changes

hannes99 force-pushed the kdb_cli_rewrite branch from 5fe0c43 to 7c82c5f Compare May 18, 2023 07:58

kodebach mentioned this pull request May 18, 2023

Move headers Part 1 #4920

Merged

19 tasks

hannes99 added 5 commits May 19, 2023 05:54

kdb-cli: rewrite get command

452d290

... and remove cpp implementation

kdb-cli: rewrite ls command

660d119

... and remove cpp implementation

kdb-cli: rewrite basename command

5c1900a

... and remove cpp implementation

kdb-cli: rewrite namespace command

b158328

... and remove cpp implementation

kdb-cli: rewrite dirname command

b59be64

... and remove cpp implementation

hannes99 force-pushed the kdb_cli_rewrite branch 2 times, most recently from 13cb196 to 3dd3e33 Compare May 19, 2023 05:17

kdb-cli: rewrite merge command

9bd1598

.. remove cpp implementation and fix #3131

hannes99 force-pushed the kdb_cli_rewrite branch from c6acb55 to 35f1c70 Compare May 19, 2023 05:20

hannes99 added 12 commits May 19, 2023 07:23

kdb-cli: rewrite set

b32571c

... and (partial) fix #3742, fix #4028, fix #1164

kdb-cli: add meta subcommand

81316ea

kdb-cli: rewrite meta-get

c691245

kdb-cli: rewrite meta-set

ef3c161

... and (partial) fix #3742

kdb-cli: rewrite meta-ls

dd4c9d9

kdb-cli: rewrite meta-show

a8314a7

kdb-cli: rewrite meta-rm

b354ed6

kdb-cli: rewrite mv

3f2b42f

.. and fix #3648

kdb-cli: rewrite rm

0977e9f

... and fix #3742

kdb-cli: rewrite cp

156aada

kdb-cli: rewrite find

3e4c4a8

kdb-cli: add rewrite release-note

6a878dc

hannes99 force-pushed the kdb_cli_rewrite branch from 16b9e77 to 4068491 Compare May 19, 2023 05:24

fix man pages

57862d8

hannes99 force-pushed the kdb_cli_rewrite branch from 4068491 to 57862d8 Compare May 19, 2023 05:31

hannes99 closed this May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kdb cli rewrite #4922

Kdb cli rewrite #4922

hannes99 commented May 11, 2023

markus2330 left a comment

markus2330 May 18, 2023

markus2330 May 18, 2023

markus2330 May 18, 2023

markus2330 May 18, 2023

markus2330 May 18, 2023

markus2330 May 18, 2023

tmakar left a comment

tmakar May 18, 2023

tmakar May 18, 2023

tmakar May 18, 2023

tmakar May 18, 2023

tmakar May 18, 2023

		- Disable cascading writes as described in #3742
		- Fix inconsistent return values #1563


		const char * metaName = GET_OPTION (options, "metaname");

		Key * toLookUp = keyNew (name, KEY_END);


		#include <kdb.h>

		/**

Kdb cli rewrite #4922

Kdb cli rewrite #4922

Conversation

hannes99 commented May 11, 2023

Basics

Checklist

Review

Labels

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

tmakar 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