Let kdb get -v display if value comes from default #2499

e01306821 · 2019-03-15T13:44:35Z

src/tools/kdb/get.cpp
I've added the check after the final key-lookup inside the execute-method. If the printTrace-method is the better place for this please let me know and I'll find a way to retrieve the information there.
Close #2474

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
I ran all tests locally and everything went fine
affected documentation is fixed
I added code comments, logging, and assertions (see doc/CODING.md)
meta data is updated (e.g. README.md of plugins)

Review

@markus2330 please review my pull request

sanssecours

I do not think I am the right person to review this pull request, since I usually do not work on anything related to the spec namespace. Anyway, as far as I can tell, the code looks good. I think it would be a good idea to create a Markdown Shell Recorder test in kdb-get.md, to make sure the feature works as intended. For an example, on how to do that please take a look at:

libelektra/doc/help/kdb-complete.md

Lines 47 to 95 in 674120e

    
           ```sh 
        
           # Backup-and-Restore: user/tests/complete/examples 
        
           # Create the keys we use for the examples 
        
           kdb set user/tests/complete/examples/kdb-complete/level1 foo 
        
           kdb set user/tests/complete/examples/kdb-complete/lvl1/lvl2 bar 
        
           kdb set user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/lvl4/lvl5 fizz 
        
           kdb set user/tests/complete/examples/kdb-complete/buzz fizzBuzz 
        
           kdb set user/tests/complete/examples/kdb-complete/#array_1 asdf 
        
           kdb set user/tests/complete/examples/kdb-complete/% nothing 
        
           # list suggestions for namespaces starting with us, only the current level 
        
           kdb complete us --max-depth=1 
        
           #> user/ 
        
           # list suggestions for namespaces starting with user, only the current level 
        
           kdb complete user --max-depth=1 
        
           #> user/ 
        
           # list suggestions for the namespace user, only the next level as it ends with / 
        
           # note the difference to the previous example, which uses no trailing / 
        
           kdb complete user/ --max-depth=1 
        
           # STDOUT-REGEX: .+ 
        
           # list all possible namespaces or mount points, only the current level 
        
           kdb complete --max-depth=1 
        
           # STDOUT-REGEX: .+ 
        
           # list suggestions for /tests/complete/examples/kdb-complete, only the current level 
        
           kdb complete /tests/complete/examples/kdb-complete --max-depth=1 
        
           #> user/tests/complete/examples/kdb-complete/ 
        
           # list suggestions for /tests/complete/examples/kdb-complete/, only the next level 
        
           # again, note the difference to the previous example which has no trailing / 
        
           kdb complete /tests/complete/examples/kdb-complete/ --max-depth=1 
        
           #> user/tests/complete/examples/kdb-complete/% 
        
           #> user/tests/complete/examples/kdb-complete/#array_1 
        
           #> user/tests/complete/examples/kdb-complete/buzz 
        
           #> user/tests/complete/examples/kdb-complete/level1 
        
           #> user/tests/complete/examples/kdb-complete/lvl1/ 
        
           # list suggestions for /tests/complete/examples/kdb-complete which are minimum 2 levels 
        
           # away from that key, and maximum 4 levels away 
        
           kdb complete /tests/complete/examples/kdb-complete/ --min-depth=2 --max-depth=4 
        
           #> user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/ 
        
           #> user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/lvl4/ 
        
           kdb rm -r user/tests/complete/examples/kdb-complete 
        
           ```

and:

libelektra/tests/shell/shell_recorder/tutorial_wrapper/CMakeLists.txt

Line 7 in 674120e

add_msr_test (kdb_complete "${CMAKE_SOURCE_DIR}/doc/help/kdb-complete.md")

.

sanssecours · 2019-03-16T08:44:29Z

doc/news/_preparation_next_release.md

@@ -173,6 +173,7 @@ you up to date with the multi-language support provided by Elektra.

 ## Tools

+- `kdb get -v` now displays if the resulting value is a default-value defined by the metadata of the key. _(Thomas Bretterbauer)_ 


Please remove the trailing space character in this line. After you do that the test testscr_check_formatting should not fail anymore.

e01306821 · 2019-03-18T09:36:13Z

Thanks for your review! I'll address your findings

Fixes ElektraInitiative#2474 Added release notes Using char-array directly Added markdown-tests and removed space in release-notes

e01306821 · 2019-03-18T14:50:32Z

I've added shell recorder tests for /doc/help/kdb-get.md which resulted in reformatting the existing examples.
The formatting-check fails because "kdb get -v" prints a trailing space-character after options:-output

Example-output:
...
searching dir/tests/get/examples/kdb-get/key, found: , options:
...

Therefore this test fails without this character. Should I ignore this failure in this case?

markus2330 · 2019-03-18T16:36:48Z

I think we can safely remove the space from the output (or even options: if there are none).

@sanssecours is this a bug in the Markdown formatter? Imho it should not reformat any source code.

sanssecours · 2019-03-18T19:24:19Z

Should I ignore this failure in this case?

I think it should work, if you

add the trailing space to the Shell Recorder test, and then
enclose the whole test in  and  tags

.

markus2330 · 2019-03-19T09:54:39Z

Alternatively, we could simply not output "options" if there are none.

e01306821 · 2019-03-19T13:47:25Z

Thanks for your help!

I've removed the options-output if there are no options to print.

@markus2330 please review my pull request

sanssecours · 2019-03-19T14:25:59Z

is this a bug in the Markdown formatter?

I do not think so.

Imho it should not reformat any source code.

In my opinion removing trailing whitespace should be a standard feature of every formatting tool 😊. The nice thing is that prettier even reformats fenced code blocks, for supported languages such as YAML and JavaScript.

please review my pull request

I do not know, if you noticed it yet, but the test testshell_markdown_kdb_get fails in the build job debian-stable-full-ini. The cause of is the INI plugin, which adds keys in between levels. I think there are multiple options to fix the failing test:

exclude the test testshell_markdown_kdb_get, if INI is the default storage plugin,
mount a plugin that “behaves properly”, or
adapt the MSR test in another way

.

sanssecours · 2019-03-20T16:10:55Z

scripts/jenkins/Jenkinsfile

+def ctest(target = "Test", testName = "Testname") {
+
+  /*
+  	 We disable the test `testshell_markdown_kdb_get` if the storage plugin INI is used, because it adds keys in between levels which


I think the proper location to disable the test is the CMake code, otherwise the test will still fail, if someone uses INI as default storage plugin and runs the test suite locally. Sorry if that was not clear from the suggestions in my previous comment.

I am also happy with the solution in this PR (if it works). I think people should not get the impression that the INI plugin works flawlessly, as it does not. Afaik nobody (except us on the build server) currently runs the tests with INI as default storage. I think soon it will be even okay to completely remove the INI-as-default tests, as we decided to use YAML and not INI as future default storage (for good reasons).

Thanks for your feedback!
First I'll try to figure out why my test still gets executed on debian-stable-full-ini. If I don't find my error, I'll look into the CMake code.

The fix in the CMake code most likely is much easier as many such fixes already exist.

See doc/todo/TESTING for a list of disabled tests. Please also add your test into this list.

e01306821 · 2019-03-21T23:43:32Z

The test now requires the dump storage plugin to run. Is that an acceptable solution?
I'm however not sure why testmod_dbus fails now.

sanssecours · 2019-03-22T07:11:07Z

The test now requires the dump storage plugin to run. Is that an acceptable solution?

It is, in my opinion 😊. It would be great, if you could mount the dump plugin at the start of the MSR test and unmount it at the end. This way you make sure that the test uses the correct storage plugin. For an example, on how to do this please take a look here.

I'm however not sure why testmod_dbus fails now.

That is a known problem. I restarted the failed build jobs.

markus2330 · 2019-03-22T10:28:47Z

Thank you very much for your contribution! Everything is now nice and clean!

markus2330 requested a review from sanssecours March 15, 2019 17:51

sanssecours removed their request for review March 16, 2019 07:21

sanssecours reviewed Mar 16, 2019

View reviewed changes

Let kdb get -v display if value comes from default

054f4a4

Fixes ElektraInitiative#2474 Added release notes Using char-array directly Added markdown-tests and removed space in release-notes

e01306821 force-pushed the fix/kdb_get_verbose_output branch from 354cb56 to 054f4a4 Compare March 18, 2019 14:13

Only print options if there are any

acf72f7

e01306821 force-pushed the fix/kdb_get_verbose_output branch from bde0793 to d0847a8 Compare March 20, 2019 15:40

sanssecours reviewed Mar 20, 2019

View reviewed changes

e01306821 force-pushed the fix/kdb_get_verbose_output branch from 7dba21e to b09022a Compare March 21, 2019 20:57

Require dump to execute kdb-get test

cf0e9f1

e01306821 force-pushed the fix/kdb_get_verbose_output branch from b09022a to cf0e9f1 Compare March 21, 2019 21:55

Mount dump to ensure its usage

3d4a51b

markus2330 merged commit 2ad71f9 into ElektraInitiative:master Mar 22, 2019

markus2330 added the ready to merge label Mar 22, 2019

markus2330 mentioned this pull request Mar 23, 2019

mmapcache #2457

Merged

9 tasks

sanssecours added enhancement configuration management labels Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let kdb get -v display if value comes from default #2499

Let kdb get -v display if value comes from default #2499

e01306821 commented Mar 15, 2019

sanssecours left a comment

sanssecours Mar 16, 2019

e01306821 commented Mar 18, 2019

e01306821 commented Mar 18, 2019

markus2330 commented Mar 18, 2019

sanssecours commented Mar 18, 2019

markus2330 commented Mar 19, 2019

e01306821 commented Mar 19, 2019

sanssecours commented Mar 19, 2019 •

edited

sanssecours Mar 20, 2019

markus2330 Mar 20, 2019

e01306821 Mar 21, 2019

markus2330 Mar 21, 2019

e01306821 commented Mar 21, 2019

sanssecours commented Mar 22, 2019

markus2330 commented Mar 22, 2019

	```sh
	# Backup-and-Restore: user/tests/complete/examples

	# Create the keys we use for the examples
	kdb set user/tests/complete/examples/kdb-complete/level1 foo
	kdb set user/tests/complete/examples/kdb-complete/lvl1/lvl2 bar
	kdb set user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/lvl4/lvl5 fizz
	kdb set user/tests/complete/examples/kdb-complete/buzz fizzBuzz
	kdb set user/tests/complete/examples/kdb-complete/#array_1 asdf
	kdb set user/tests/complete/examples/kdb-complete/% nothing

	# list suggestions for namespaces starting with us, only the current level
	kdb complete us --max-depth=1
	#> user/

	# list suggestions for namespaces starting with user, only the current level
	kdb complete user --max-depth=1
	#> user/

	# list suggestions for the namespace user, only the next level as it ends with /
	# note the difference to the previous example, which uses no trailing /
	kdb complete user/ --max-depth=1
	# STDOUT-REGEX: .+

	# list all possible namespaces or mount points, only the current level
	kdb complete --max-depth=1
	# STDOUT-REGEX: .+

	# list suggestions for /tests/complete/examples/kdb-complete, only the current level
	kdb complete /tests/complete/examples/kdb-complete --max-depth=1
	#> user/tests/complete/examples/kdb-complete/

	# list suggestions for /tests/complete/examples/kdb-complete/, only the next level
	# again, note the difference to the previous example which has no trailing /
	kdb complete /tests/complete/examples/kdb-complete/ --max-depth=1
	#> user/tests/complete/examples/kdb-complete/%
	#> user/tests/complete/examples/kdb-complete/#array_1
	#> user/tests/complete/examples/kdb-complete/buzz
	#> user/tests/complete/examples/kdb-complete/level1
	#> user/tests/complete/examples/kdb-complete/lvl1/

	# list suggestions for /tests/complete/examples/kdb-complete which are minimum 2 levels
	# away from that key, and maximum 4 levels away
	kdb complete /tests/complete/examples/kdb-complete/ --min-depth=2 --max-depth=4
	#> user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/
	#> user/tests/complete/examples/kdb-complete/lvl1/lvl2/lvl3/lvl4/

	kdb rm -r user/tests/complete/examples/kdb-complete
	```

		@@ -173,6 +173,7 @@ you up to date with the multi-language support provided by Elektra.

		## Tools

		- `kdb get -v` now displays if the resulting value is a default-value defined by the metadata of the key. _(Thomas Bretterbauer)_

Let kdb get -v display if value comes from default #2499

Let kdb get -v display if value comes from default #2499

Conversation

e01306821 commented Mar 15, 2019

Checklist

Review

sanssecours left a comment

Choose a reason for hiding this comment

sanssecours Mar 16, 2019

Choose a reason for hiding this comment

e01306821 commented Mar 18, 2019

e01306821 commented Mar 18, 2019

markus2330 commented Mar 18, 2019

sanssecours commented Mar 18, 2019

markus2330 commented Mar 19, 2019

e01306821 commented Mar 19, 2019

sanssecours commented Mar 19, 2019 • edited

sanssecours Mar 20, 2019

Choose a reason for hiding this comment

markus2330 Mar 20, 2019

Choose a reason for hiding this comment

e01306821 Mar 21, 2019

Choose a reason for hiding this comment

markus2330 Mar 21, 2019

Choose a reason for hiding this comment

e01306821 commented Mar 21, 2019

sanssecours commented Mar 22, 2019

markus2330 commented Mar 22, 2019

sanssecours commented Mar 19, 2019 •

edited