Skip to content
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

Add is_choice to test entry in list #102

Closed
wants to merge 10 commits into from

Conversation

stdweird
Copy link
Member

@stdweird stdweird commented Feb 4, 2016

Use case is in schema files to use

    "attribute" ? string with is_choice("a", "b", "c")

instead of current with match(SELF, ''^(a|b|c)$")

(and it also allows string[] with is_choice(...) transparently)

@stdweird
Copy link
Member Author

stdweird commented Feb 4, 2016

This PR is opened without any actual is_choice code
@jrha the .tests location is temporary

@stdweird stdweird mentioned this pull request Feb 4, 2016
# Merge the arguments into the given array. Neither the
# first/next or merge functions can be used because the
# ARGV array cannot be directly referenced.
# Cannot use merge or refernce ARGV directly
i = 0;
while (i<ARGC) {
v[length(v)] = ARGV[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be replaced with append(ARGV[i]) ? That's what I deployed to our code base internally recently.

@stdweird
Copy link
Member Author

stdweird commented Feb 7, 2016

@ned21 from the comments in the old code, it looked that ARGV was some special construct and couldn't be used with merge etc. however, that doesn't seem to be the case in pan 10.2 (and also stated as such in the pan docs). see b1b170c

what release of panc can we use to release the core templates?
also, can you have a good look at the unit-tests to see what can be added to make sure the new code is correctly behaving?

function full_hostname_from_object = {
# Check cardinality; leave detailed error checking to called functions.
if (ARGC != 1) error("usage: full_hostname_from_object(default_domain)");
if (ARGC != 1) error("usage: full_hostname_from_object(default_domain) requires default_domian as argument");
Copy link
Contributor

Choose a reason for hiding this comment

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

default domian -> default domain

@jouvin
Copy link
Contributor

jouvin commented Feb 8, 2016

@stdweird about merge() and ARGV, my guess it that this is a very old comment (this template if probably coming from ancient times without being significantly cleaned up...). As for the minimal panc version required by the template library (not only the core part), we had a discussion about this in the last year. I was not able to find the issue where it was discussed but I think we agreed than 10.2 was the minimum version required (as there is no reason to stay with 10.0 or 10.1 and 10.2 was brinig important improvements for metaconfig (to help support of variable names).

############################################################
@documentation{
pushes zero of more pairs (key, value) into a
dict. If the list does not exist or is not defined
Copy link
Contributor

Choose a reason for hiding this comment

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

s/list/dict/ in this line

@jouvin
Copy link
Contributor

jouvin commented Nov 8, 2016

Any good reason for this PR not to be merged yet... Would be good to have in 16.10 so that we can start using it,..

@stdweird
Copy link
Member Author

stdweird commented Nov 8, 2016

this PR is completely irrelevant if someone could be bothered to review merge quattor/pan#121 and quattor/pan#125 ...

@jrha
Copy link
Member

jrha commented Dec 7, 2016

Those two PRs are now merged, so this probably needs reworking.

@jrha
Copy link
Member

jrha commented Jun 8, 2018

@stdweird is this now irrelevant?

@jrha
Copy link
Member

jrha commented Jul 25, 2018

@stdweird ping?

@stdweird stdweird closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants