New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canonical: Preview #1539

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@tom-wa
Contributor

tom-wa commented Jul 15, 2017

Purpose

Preview: Canonicalization plugin - matches keyvalue against either a list of values, posix extended regex or fnmatch and on a match brings the value to a canonical form

Some examples are in README.md

Pending decisions:

  • move stringToArray helper function to a library ?
  • compilation variants for regex, lists and fnmatch ?
  • per mountpoint configurations ?

TODO:

  • error handling
  • cleanup
  • Documentation

Checklist

  • commit messages are fine (with references to issues)
  • I ran all tests and everything went fine
  • I added unit tests
  • affected documentation is fixed
  • I added code comments, logging, and assertions
  • meta data is updated (e.g. README.md of plugins)

@markus2330 please review my pull request

@tom-wa tom-wa force-pushed the tom-wa:canonical branch from 08dd075 to 1e35cc7 Jul 16, 2017

@tom-wa tom-wa force-pushed the tom-wa:canonical branch from 1e35cc7 to bc05c1f Jul 16, 2017

@markus2330

I am impressed, much more clean and readable code! I only found minor things.

What about having a preconfigured way to get canonicalization like done in the plugin bool? Somewhere in the future we could even let the plugin bool depend on this plugin. (We should not do it now, it has advantages that the plugin bool is self contained.

## Introduction
Canonicalizes keyvalues matching a lists of valid values, posix extended regex or fnmatch(3) pattern.

This comment has been minimized.

@markus2330

markus2330 Jul 20, 2017

Contributor

The word "glob" is more known than fnmatch; you should mention it.

array = elektraCalloc (elems * sizeof (char *));
char * localString = NULL;
if (string[0] == '[' || string[0] == '(' || string[0] == '{')
localString = elektraStrDup (string + 1);

This comment has been minimized.

@markus2330

markus2330 Jul 20, 2017

Contributor

minor: you could avoid the else branch by calculating an offset (which is either 0 or 1)

Please prefer to use {}.

if (*ptr == '\0')
{
ptr = strtok (NULL, delim);
continue;

This comment has been minimized.

@markus2330

markus2330 Jul 20, 2017

Contributor

minor: you could easily avoid continue by putting the rest in the else branch.

char * start = elektraLskip (tmp);
(void)elektraRstrip (start, NULL);
if (start[0] == '\'') ++start;
if (start[strlen (start) - 1] == '\'') start[strlen (start) - 1] = '\0';

This comment has been minimized.

@markus2330

markus2330 Jul 20, 2017

Contributor

Please use {}, its easy to get confused here where []() start/stop.

@tom-wa

This comment has been minimized.

Contributor

tom-wa commented Jul 20, 2017

What about having a preconfigured way to get canonicalization like done in the plugin bool?

i'm currently trying to figure out which approach of preconfiguring works best: plugin configuration or the way typedispatcher currently does it (mounting a configuration and letting the plugin read it)
the latter one seems easier for everything thats more than 3 configuration keys to set.
should we give plugins a way to automatically fetch configurations below some specified mountpoint on kdbOpen ?

@markus2330

This comment has been minimized.

Contributor

markus2330 commented Jul 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment