[FLOSS P3] Kdb validate #4224

flo91 · 2022-01-12T06:26:02Z

Basics

Related issue: #3674

These points need to be fulfilled for every PR:

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 something 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
The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

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
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 THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

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

If you are already Elektra developer:

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and you also
say that everything is ready to be merged.

markus2330

Very comprehensive work, looks very good to reach all points. 🚀 Please focus on reaching a mergable state.

markus2330 · 2022-01-18T16:53:12Z

doc/help/kdb-validate.md

+## OPTIONS
+
+- `-d`,`--debug`:
+  Give debug information or ask debug questions (in interactive mode).


What is the interactive mode? There doesn't seem to be a -i option?

Thanks for reporting that mistake!
That was a leftover from a copy-paste of the manpage from another tool.
Gets fixed by commit c2ec7eb.

markus2330 · 2022-01-18T16:53:30Z

doc/help/kdb-validate.md

+- `-d`,`--debug`:
+  Give debug information or ask debug questions (in interactive mode).
+- `-f`, `--force`:
+  Force the action to be done.


Suggested change

Force the action to be done.

Force writing the configuration even on warnings.

Thanks, good clarification!
Gets changed by commit c2ec7eb.

markus2330 · 2022-01-18T16:58:57Z

src/tools/kdb/validate.cpp

+		throw invalid_argument ("1 argument needed");
+	}
+
+	cout << "The given path was: " << cl.arguments[0] << endl;


Output only on debug (if at all). Probably more interesting is the name of the root key and not getting back exactly the given argument.

Thx, I fixed it while adapting the ValidateCommand::execute() to use the new C++ error-classes.

markus2330 · 2022-01-18T17:00:53Z

src/tools/kdb/validate.cpp

+
+		if (cl.force)
+		{
+			cout << getFormattedInfoString (


again: only for -v.

Fixed by commit 18edf39.

markus2330 · 2022-01-18T17:01:03Z

src/tools/kdb/validate.cpp

+					"The validation was stopped because of warnings "
+					"while getting the values!")
+			     << endl;
+			kdb.close ();


Is done automatically.

Thx, I removed the statement.

markus2330 · 2022-01-18T17:09:45Z

src/libs/tools/include/errors/error.hpp

+	 * The warning is copied to make it independent from the source object. This way the same warning added to two different errors can be
+	 * changed independently.
+	 *
+	 * An Error can contain 0 to n warnings.


A key can contain 0-n warnings. I would say that the error does.

Here we adhered to the concept that was established by the Error-Keys and also the highlevel-API (/src/libs/highlevel/elektra_error.c).
Therefore we designed an Error-class that can hold multiple Warnings, so a Key of the C-API with 0 or 1 errors and 0 to n warnings can be directly converted to an object of type Error and the behaviour is consistent with the highlevel-API.

I extended the comment to explain the concept a bit in commit c2ec7eb.

markus2330 · 2022-01-18T17:10:35Z

src/libs/tools/include/errors/errorTypes.hpp

+namespace errors
+{
+
+/* Not an error by itself, but a container for multiple warnings */


Suggested change

/* Not an error by itself, but a container for multiple warnings */

/* Not an warning by itself, but a container for multiple warnings */

As I wrote in the previous message, an Error object can contain multiple Warnings so that an Error-key can directly be converted to a single C++ Error-object and to keep the behaviour consistent with the highlevel-API.
In commit c2ec7eb I extended the comment to explain the concept a bit more.

markus2330 · 2022-01-18T17:12:43Z

src/libs/tools/include/errors/errorTypes.hpp

+
+
+/* Concrete classes for the different Errors, based on the constants defined in /src/include/kdberrors.h */
+class ResourceError : public Error


Actually this could even be in src/bindings/cpp/include/kdbexcept.hpp this would need some exception mapping in src/bindings/cpp/include/kdb.hpp, though. (extra work beyond original scope of project)

markus2330 · 2022-01-18T17:14:35Z

src/libs/tools/src/errors/error.cpp

+		{
+			/*TODO: Decide if ordering of warnings should be considered for equality.
+			 * Currently two errors are equal if they contain the same warnings (compared by member values),
+			 * even if they have different orders in the internal vector.*/


Sounds good.

Thank you for the good feedback! 😀

markus2330 · 2022-01-18T17:15:41Z

src/libs/tools/src/errors/errorFactory.cpp

+Error* ErrorFactory::create(const std::string & type, const std::string & reason, const std::string & module,
+				  const std::string & file, const std::string & mountPoint, const std::string & configFile, kdb::long_t line)
+{
+	if (type == ELEKTRA_ERROR_RESOURCE || type == ELEKTRA_ERROR_RESOURCE_NAME)


Why would be the type sometimes the _NAME and sometimes not?

At the bottom of /src/include/kdberrors.h, for every introduced code four constants are defined:

#define DECLARE_ERROR_CODE(cname) \ extern const char * ELEKTRA_ERROR_##cname; \ extern const char * ELEKTRA_ERROR_##cname##_NAME; \ \ extern const char * ELEKTRA_WARNING_##cname; \ extern const char * ELEKTRA_WARNING_##cname##_NAME; \ \ void elektraSetError##cname (Key * key, const char * file, const char * line, const char * module, const char * reason, ...); \ void elektraAddWarning##cname (Key * key, const char * file, const char * line, const char * module, const char * reason, ...); DECLARE_ERROR_CODE (RESOURCE) DECLARE_ERROR_CODE (OUT_OF_MEMORY) DECLARE_ERROR_CODE (INSTALLATION) DECLARE_ERROR_CODE (INTERNAL) DECLARE_ERROR_CODE (INTERFACE) DECLARE_ERROR_CODE (PLUGIN_MISBEHAVIOR) DECLARE_ERROR_CODE (CONFLICTING_STATE) DECLARE_ERROR_CODE (VALIDATION_SYNTACTIC) DECLARE_ERROR_CODE (VALIDATION_SEMANTIC) #undef DECLARE_ERROR_CODE

We designed the Factories so that the ELEKTRA_ERROR_##cname or ELEKTRA_ERROR_##cname##_NAME are valid parameters for creating an object of the appropriate type.

Should we change that so only the Code, but not the Description/_NAME is valid here?

Yes, I do not think that _NAME should be anything external. It is only needed to render the error text. I created #4230

@markus2330

…ektraInitiative#4224 by @markus2330

@markus2330

…ktraInitiative#4224 by @markus2330

and do some improvements and bugfixes esp. regarding handling of warnings

currently mostly a wrapper to the implementation in /src/libs/highlevel/elektra_error.c not finished or in a working state yet!

of the highlevel API

add iterator functionality for Warnings to Error class

…warnigns Warnings get now cloned when they are stored in an Error object, add further tests

add MountPoint and ConfigFile members, add a PureWarning ErrorType for a dummy Error that only encapsulats Warnings, add some testcode to /src/tools/kdb/validate.cpp

@markus2330

…ktraInitiative#4224 by @markus2330

…f was set only when also -v was given

… manually - they are currently broken!)

…by the CI

libtools: make error and warning factories not instantiable (by deleting their default constructors and destructors)

markus2330 · 2022-01-24T14:33:26Z

Excellent job! 💖

flo91 added cm2022s for university course lang/cpp project2022w Suitable as project work in progress labels Jan 12, 2022

flo91 assigned JakobWonisch and flo91 Jan 12, 2022

flo91 changed the title ~~[FLOSS P3] Kdb validate~~ [FLOSS P3 - WIP!] Kdb validate Jan 12, 2022

flo91 force-pushed the kdb_validate branch 2 times, most recently from 17480a0 to ae1ae3e Compare January 16, 2022 14:29

markus2330 reviewed Jan 18, 2022

View reviewed changes

flo91 added a commit to flo91/libelektra that referenced this pull request Jan 19, 2022

libtools: fix or clearify most aspects that were reviewd in the PR El…

054a25f

…ektraInitiative#4224 by @markus2330

flo91 added a commit to flo91/libelektra that referenced this pull request Jan 19, 2022

libtools: fix or clarify most aspects that were reviewd in the PR Ele…

c2ec7eb

…ktraInitiative#4224 by @markus2330

flo91 force-pushed the kdb_validate branch from 054a25f to c2ec7eb Compare January 19, 2022 17:32

flo91 and others added 16 commits January 19, 2022 23:29

kdb: implement prototype of kdb validate <key>

22c09ff

and do some improvements and bugfixes esp. regarding handling of warnings

libtools: WIP! - add draft for Error/Warning API (binding) for C++

fb1bc28

currently mostly a wrapper to the implementation in /src/libs/highlevel/elektra_error.c not finished or in a working state yet!

libtools: Work on C++ Error/Warnings API, wrapper for elektra_error.c

936daa0

of the highlevel API

libtools: extend and refactor error/warning API, add some basic tests,

1eaebfc

add iterator functionality for Warnings to Error class

libtools: warnings/errors API, add comparing of ojbects & cloning of …

8b662a1

…warnigns Warnings get now cloned when they are stored in an Error object, add further tests

libtools: add Factories for Errors and Warnings,

100b8d2

add MountPoint and ConfigFile members, add a PureWarning ErrorType for a dummy Error that only encapsulats Warnings, add some testcode to /src/tools/kdb/validate.cpp

doc: add kdb validate tutorial

ea3c01d

doc: add kdb validate man page

e2e6380

doc: fix kdb validate shell recorder test

f415a62

doc: add doxygen descriptions

542fe70

doc: move doxygen to header files

ea62646

libtool: remove small unnecassary comment

2552170

doc: add string keys only to validate documentation

ddb59fd

doc: add string keys only to doxygen as well

8974542

libtools: fix or clarify most aspects that were reviewd in the PR Ele…

9894405

…ktraInitiative#4224 by @markus2330

libtools: bring the branch into a working state

06d5ccd

flo91 force-pushed the kdb_validate branch from bebb62a to ba673e0 Compare January 19, 2022 22:29

flo91 marked this pull request as ready for review January 19, 2022 22:29

flo91 changed the title ~~[FLOSS P3 - WIP!] Kdb validate~~ [FLOSS P3] Kdb validate Jan 19, 2022

libtools: reformat code, close ElektraInitiative#3674

9daf801

flo91 force-pushed the kdb_validate branch from ba673e0 to 9daf801 Compare January 19, 2022 22:30

flo91 removed the work in progress label Jan 19, 2022

flo91 added 3 commits January 19, 2022 23:40

kdb validate: info output when warnigns occured while kdb.get() and -…

18edf39

…f was set only when also -v was given

doc: update links in manpages as reported by the buildserver check

937b329

doc: remove broken links as reported by the buildserver (checked also…

c8525a7

… manually - they are currently broken!)

flo91 force-pushed the kdb_validate branch from 41cf1df to c8525a7 Compare January 19, 2022 23:03

flo91 added 3 commits January 20, 2022 13:24

doc: create manpage and remove broken links from READMEs as reported …

b498283

…by the CI

doc: patch manpage for changed readme

b0f940c

kdb: fix error so that -f without -v is still correctly handled

4cab540

libtools: make error and warning factories not instantiable (by deleting their default constructors and destructors)

flo91 added the ready to merge label Jan 21, 2022

markus2330 merged commit 93c3f0d into ElektraInitiative:master Jan 24, 2022

markus2330 mentioned this pull request Jan 25, 2022

remove *_NAME from public symbols #4230

Closed

mpranj added this to the 0.9.9 milestone Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLOSS P3] Kdb validate #4224

[FLOSS P3] Kdb validate #4224

flo91 commented Jan 12, 2022 •

edited

Loading

markus2330 left a comment

markus2330 Jan 18, 2022

flo91 Jan 19, 2022 •

edited

Loading

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022

markus2330 Jan 18, 2022

flo91 Jan 19, 2022 •

edited

Loading

markus2330 Jan 25, 2022

markus2330 commented Jan 24, 2022

	Force the action to be done.
	Force writing the configuration even on warnings.

	/* Not an error by itself, but a container for multiple warnings */
	/* Not an warning by itself, but a container for multiple warnings */



		/* Concrete classes for the different Errors, based on the constants defined in /src/include/kdberrors.h */
		class ResourceError : public Error

[FLOSS P3] Kdb validate #4224

[FLOSS P3] Kdb validate #4224

Conversation

flo91 commented Jan 12, 2022 • edited Loading

Basics

Checklist

Review

Labels

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo91 Jan 19, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

flo91 Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 commented Jan 24, 2022

flo91 commented Jan 12, 2022 •

edited

Loading

flo91 Jan 19, 2022 •

edited

Loading

flo91 Jan 19, 2022 •

edited

Loading