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

(PUP-4178) Move defined function to 4.x to correct behavior #3840

Merged
merged 7 commits into from Apr 22, 2015

Conversation

hlindberg
Copy link
Contributor

Before this, the 3x defined() function beahaves in a strange way when
giving undef and empty string as input. A user may want to check if a
variable is defined and calls:

defined($not_defined)

and becomes surprised when true is returned even if the variable is not
defined. The user should have asked for:

defined('$not_defined')

This anomaly is caused by the 3.x functions inability to distinquish
between undef and an empty string, and the strange naming of the
Class[main] which is called '' (the empty string) internally, and 'main'
externally.

This commit fixes this problem by adding a 4.x implementation of the
defined() function. This version:

  • does not accept undef input (protects users from the mistake of
    defined($foo)).
  • the empty string produces false
  • the string 'main' produces true (note that there is always a main
    class)

The documentation for the 3x defined function is also updated to reflect
this (to avoid confusion).

This also fixes the ticket PUP-4331 since what it suggest (giving an
error when given undef) is implmented by typing the arguments to the
function.

@puppetcla
Copy link

CLA signed by all contributors.

@hlindberg hlindberg force-pushed the PUP-4178_add-defined-4x-function branch from 8985a73 to 592b332 Compare April 17, 2015 15:47
@hlindberg
Copy link
Contributor Author

Hm problems with Ruby 1.8.7. And this problem actually unveiled a different issue... brb

@hlindberg hlindberg force-pushed the PUP-4178_add-defined-4x-function branch from e94eb6b to 3ade911 Compare April 18, 2015 00:35
@thallgren
Copy link
Contributor

Suggest deferring this until PUP-4438 has been fixed.

@hlindberg hlindberg force-pushed the PUP-4178_add-defined-4x-function branch from 3ade911 to 830c47c Compare April 21, 2015 01:04
@hlindberg
Copy link
Contributor Author

This PR was rebased and the new functionality from pup 4438 was used. A problem was found with weaving and repeated parameter. I added a fix for that (commit e9787bf in this PR) and could then use the new feature.

# defined(Class['foo'])
#
# The `defined` function does not answer if data types (e.g. `Integer`) are defined.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it answer on datatypes then? false or true?

Before this, the 3x defined() function beahaves in a strange way when
giving undef and empty string as input. A user may want to check if a
variable is defined and calls:
```
defined($not_defined)
```
and becomes surprised when true is returned even if the variable is not
defined. The user should have asked for:
```
defined('$not_defined')
```
This anomaly is caused by the 3.x functions inability to distinquish
between undef and an empty string, and the strange naming of the
Class[main] which is called '' (the empty string) internally, and 'main'
externally.

This commit fixes this problem by adding a 4.x implementation of the
defined() function. This version:
* does not accept undef input (protects users from the mistake of
  defined($foo)).
* the empty string produces false
* the string 'main' produces true (note that there is always a main
class)

The documentation for the 3x defined function is also updated to reflect
this (to avoid confusion).

This also fixes the ticket PUP-4331 since what it suggest (giving an
error when given undef) is implmented by typing the arguments to the
function.
Before this, the new 4.x function would report that non included
classes were defined. This problem does not exist in the 3x version
since class references are transformed to Resource references.

The code path for Class was simply never exercised, abut when moved to
4.x. the faulty check if class existed was used instead of a check if it
was defined (i.e. realized).

This changes the 4.x function to lookup the class resouce instead of the
class definition.
Ruby 1.8.7 have a problem with turning empty string into a sym.

This problem uncovered yet another problem; the logic used
scope.find_class which is not what is expected (as it returns true
if a class exists but is not included).
Before this, the tests were not covering all variants. This commit
organizes the spec test for the defined function from a usage
perspective. To ask if a class is defined, if a resource is defined, a
variable etc. Tests were also added for numeric (match) variables.

This revealed that it was not possible to be specific about if a name
must be a reference to a class, defined or resource type.

This commit also adds the ability to be specific by using a meta type.
Thus, defined(Type[Class[foo]]) returns true iff foo is a class, and
defined(Type[Resource[foo]]) returns true iff foo is a build in or
defined resource type.
Before this, if weaving was used and there was a repeated parameter
it would only receive the first value in the set of variable values.
This was caused by recording the index of the parameter, and then
picking only this argument when weaving.

This fix records repeating parameters as a negative value where -1
means value 0..-1, and thus -n means values (-n-1)..-1. The weaving then
weaves a new array where multiple values are copied over.
Before this, if an error was made when giving arguments the error
message would indicate a required parameter followed by 0:M additional
parameters. This is bad because conceptually it is "one or more" values.
@hlindberg hlindberg force-pushed the PUP-4178_add-defined-4x-function branch from 830c47c to 5d5771c Compare April 21, 2015 15:59
@hlindberg
Copy link
Contributor Author

@thallgren review-comments addressed, rebased on top of change to check that required does not follow optional

next nil
when 'main'
# Find the main class (known as ''), it does not have to be in the catalog
scope.find_hostclass('') # scope.find_hostclass('')
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment on the same line seems somewhat redundant.

Stylistic issues corrected
Use of required_repeated_param instead of optional_repeated_param
Documentation clarifications
@hlindberg hlindberg force-pushed the PUP-4178_add-defined-4x-function branch from 5d5771c to c366fd3 Compare April 21, 2015 20:25
thallgren added a commit that referenced this pull request Apr 22, 2015
(PUP-4178) Move defined function to 4.x to correct behavior
@thallgren thallgren merged commit 06aab91 into puppetlabs:3.x Apr 22, 2015
@hlindberg hlindberg deleted the PUP-4178_add-defined-4x-function branch September 16, 2017 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants