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

conditionals: now with topological sort #907

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tom-wa
Contributor

tom-wa commented Aug 31, 2016

Purpose

Added topological sorting

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)
@@ -785,6 +787,129 @@ static CondResult evaluateKey (const Key * meta, const Key * suffixList, Key * p
return TRUE;
}
static KeySet * getDependencies (const char * expr, Key * cur, Key * parentKey)
{
char * localExpr = strdup (expr);

This comment has been minimized.

@markus2330

markus2330 Sep 2, 2016

Contributor

please use elektraStrDup.

KeySet * deps = ksNew (0, KS_END);
int isEscaped = 0;
int hasQuote = 0;
for (; *ptr; ++ptr)

This comment has been minimized.

@markus2330

markus2330 Sep 2, 2016

Contributor

Shouldn't this parsing code be part of libmeta (later)? Some generic link detection would be nice.

@markus2330

This comment has been minimized.

Contributor

markus2330 commented Sep 2, 2016

Please add unit tests. What else is missing?

@tom-wa

This comment has been minimized.

Contributor

tom-wa commented Sep 6, 2016

Shouldn't this parsing code be part of libmeta (later)?

you are right. currently conditionals is the only plugin that benefits from it though, so let's keep it that way for now, i'll improve it and move it to libmeta later.

What else is missing?

a lot of unit tests, +a few things to discuss:
for now, should the topological sort be used by default or activated by a flag (until it's proven to work) ?
what should be done if there are cyclic dependencies or invalid dependencies ?

@markus2330

This comment has been minimized.

Contributor

markus2330 commented Sep 7, 2016

should the topological sort be used by default

I think we should not do this for the next release anyway, but the idea is that it should be default when any calculations are involved.

In longer term we should split the plugin to a validation and a calculation plugin. The calculation should be done at lookup time (see #867). Only the calculation needs correct ordering, for validation the order only matters for better error messages, but is not needed for correctness.

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