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

CSVStorage and Lineendings #298

Closed
wants to merge 15 commits into from
Closed

Conversation

tom-wa
Copy link
Contributor

@tom-wa tom-wa commented Sep 17, 2015

No description provided.

\[not a section\]=yeah
loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooongggggggg=truncated
something = ; nothing
a = \777
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be commited.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still todo

## Introduction ##

The Lineendings Plugin verifies the Lineendings of a file.
If inconsistent lineendings or lineendings that don't match `config/valid` are detected the plugin yields an error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, no config/

Can you explain valid a bit more? Does it contain a sequence of lineendings? If \r\r\f is given, how does it know if \r\f should be accepted?

Can you give an example how to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid could be set to CR, LF or CRLF (string value) to tell the plugin to reject all other lineendings. without the valid key, only inconsistent lineendings are rejected
gonna update the readmes

@markus2330
Copy link
Contributor

Overall very nice and clean code, impressive work. Thank you!

@markus2330
Copy link
Contributor

Two more remarks:

  • you can remove what you have done from the TODO lists
  • please add the plugins in cmake/ElektraCache

@markus2330
Copy link
Contributor

add to whitelist

@markus2330
Copy link
Contributor

jenkins build please


## Usage ##

The plugin checks every Key in the Keyset for the Metakey `check/enum` containing a list with the syntax `['string1', 'string2', 'string3', ..., 'stringN']` and compares each value with the string value of the Key. If no match is found an error is returned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the syntax: Why is it surrounded with []? It actually does make sense if you also can specify an enum somewhere else (e.g. in the plugin configuration) and reuse it check/enum (then without square brackets).

I am wondering how fast the check will perform with an enum of 700 values, copied to 700 places? A reference to the plugins configuration, and not copying the enum values, is certainly faster, but maybe its a premature optimization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And more about the syntax: how is it possible to use ' within the string?

One way would be escaping with \, another one would be to provide different quotes (e.g. ' and") and automatically concatinate them, e.g.:
['"' "str'ing" '"', "abc"] would be "str'ing" and abc. Then the usage of , to separate would pay off. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, the [] are not even needed when refering to outside defined enums is allowed. You simply know if you refer to something else, when its not quoted in '.

So given a defined abc = 'a', 'b', 'c' one can use check/type with abc, 'd' without ambiguity. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [] isn't needed,i was trying to adapt the syntax of the specification file. Basically every alphanumeric string divided by an non-alphanumeric character is considered as a valid element right now, but i can change it to just use a comma as a valid seperator, values in <> to keys and not quoted values to plugin configured values ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the <> part i meant something like:
system/configs/enums/valid1='a','b','c'
system/configs/enums/valid2='g','h','i'

And the check/enum meta key could look like <enums/valid1>, 'd','e','f',<enums/valid2>

So the plugin would check for a,b,c,d,e,f,g,h

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have as little syntactic clutter as possible (aka not having
<>). Syntax should be introduced in the storage plugins, not within values.

One could even argue to introduce an array:

check/enum
check/enum/#0=a
check/enum/#1=b
check/enum/#1=c

Where a, b and c are literals, e.g. serialized in json:

{ "check" : { "enum" : ["a", "b", "c"] } }

Obviously, the data structure would be huge, we would not have nice references
(without introducing syntax) and you would need to reimplement most things.
So the 'a', 'b' for now and var, 'a', 'b', "'" for later seems like a good
middle ground.

Btw. I used JSON as example, because it looks nearly identical to your initial proposal.

@markus2330
Copy link
Contributor

Please ping me when you are ready.

@markus2330
Copy link
Contributor

jenkins build please

@markus2330
Copy link
Contributor

404 buildjob http://build.libelektra.org:8080/job/elektra-mergerequests/404/console reports
http://build.libelektra.org:8080/job/elektra-mergerequests/ws/build/Testing/20150921-1702/DynamicAnalysis.xml

==10564== 3 bytes in 1 blocks are indirectly lost in loss record 1 of 58
==10564==    at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==10564==    by 0x4E904CB: elektraStrNDup (internal.c:306)
==10564==    by 0x4E89525: keySetMeta (keymeta.c:489)
==10564==    by 0x4F17688: elektraAddWarning118 (kdberrors.h:7671)
==10564==    by 0x4F17688: csvRead (csvstorage.c:207)
==10564==    by 0x4F17688: elektraCsvstorageGet (csvstorage.c:267)
==10564==    by 0x4025E3: testreadwriteinvalid (testmod_csvstorage.c:48)
==10564==    by 0x4025E3: main (testmod_csvstorage.c:65)
==10564== 
==10564== 3 bytes in 1 blocks are indirectly lost in loss record 2 of 58

if(readHeaderKey)
{
const char *printHeaderString = keyString(readHeaderKey);
printf("printHeaderString: %s\n", printHeaderString);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed

@markus2330
Copy link
Contributor

Merged, please do not work on this branch.

Leftovers are documented in #306, but are not urgent. (Feel free to add more)

Thank you! Impressive work!

@markus2330 markus2330 closed this Sep 22, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants