Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Directory Value Plugin & Minor Fixes #1722

Merged
merged 64 commits into from
Dec 13, 2017

Conversation

sanssecours
Copy link
Member

@sanssecours sanssecours commented Dec 10, 2017

Purpose

This pull request adds a new plugin that adds a leaf (key without children) for every directory (key with children) that stores data. For more information, please take a look at issue #106.

The PR also fixes problems with the ERROR directive in the Markdown Shell Recorder, and other minor issues.

Remark

The Directory Value plugin currently does not work in conjunction with the INI plugin. I did not look closer into this issue since the Directory Value plugin seems to deliver the correct key set to the INI plugin. The code below shows a failing example:

kdb mount config.ini /examples/ini ini directoryvalue

kdb set /examples/ini/aimee/man 'Deathly'
kdb set /examples/ini/aimee 'Save Me'

kdb get /examples/ini/aimee/man
#> Deathly
kdb get user/examples/ini/aimee # 😭
#>

Checklist

  • I checked all commit messages twice.
  • I added unit tests.
  • I ran all tests and everything went fine.
  • The source of the Directory Value plugin contains a ReadMe describing the features of the plugin.
  • I added code comments, logging, and assertions.
  • The metadata of the Directory Value plugin stored in src/plugins/directoryvalue/README.md should be correct.
  • I added a description about the Directory Value plugin to the release notes.

@sanssecours
Copy link
Member Author

jenkins build all please

@markus2330
Copy link
Contributor

Build finished. No test results found.

1 similar comment
@markus2330
Copy link
Contributor

Build finished. No test results found.

@markus2330
Copy link
Contributor

markus2330 commented Dec 13, 2017

Thank you for the many improvements and the many new shell recorder snippets! You used very creative names.

I have following questions (maybe they are separate issues):

  • One of the goals of directoryvalue plugin was to avoid special handling within the testcases. For example tests/shell/check_import.sh line 58. Are there still errors if these tests run unconditionally?
  • Has there been a bug in elektraArrayValidateName? If so, please add regression tests for the bug.
  • What happens with nested directory values? (I did not see it in the docu.)
  • How can we escape ___dirdata? (Allow keys and values already named this way?)

@markus2330 markus2330 merged commit 68eb32b into ElektraInitiative:master Dec 13, 2017
@sanssecours sanssecours deleted the 🍁 branch December 13, 2017 11:08
@sanssecours
Copy link
Member Author

sanssecours commented Dec 13, 2017

One of the goals of directoryvalue plugin was to avoid special handling within the testcases. For example tests/shell/check_import.sh line 58. Are there still errors if these tests run unconditionally?

As far as I can tell the test now also works, if we remove the special case for the YAJL plugin.

Has there been a bug in elektraArrayValidateName?

Not as far as I can tell.

What happens with nested directory values? (I did not see it in the docu.)

Is:

user/grandparent                = Grandparent
user/grandparent/leaf           = Leaf
user/grandparent/parent         = Parent
user/grandparent/parent/child   = Child

(line 24-27 of the ReadMe) not already an example for nested directory values? After the conversion the key set looks like this:

user/grandparent                    =
user/grandparent/___dirdata         = Grandparent
user/grandparent/leaf               = Leaf
user/grandparent/parent             =
user/grandparent/parent/___dirdata  = Parent
user/grandparent/parent/child       = Child

. You can also see this conversion in ReadMe.md.

How can we escape ___dirdata? (Allow keys and values already named this way?)

Escaping is currently not possible. You can not use the name ___dirdata as the last part of a normal key if you use the Directory Value plugin! Escaping support is not really worth the trouble in my opinion. Are there really people who want to use a normal key that ends with ___dirdata?

@markus2330
Copy link
Contributor

As far as I can tell the test now also works, if we remove the special case for the YAJL plugin.

Thank you. You do not need to answer such questions but only point to PR/issue where it is fixed or tracked.

Not as far as I can tell.

Okay, I wondered about the change.

(line 24-27 of the ReadMe)

Sorry, I overlooked this.

Are there really people who want to use a normal key that ends with ___dirdata?

I do not know. But we should make it a habit to properly escape everything and never assume anything about the content people want to store. (Think of storing wifi SSIDs. Then you could lock out users using Elektra when you call your access point ___dirdata.)

For now it is enough if you document it as limitation.

@markus2330
Copy link
Contributor

Build finished. No test results found.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants