The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Feature #14908

Function str2bool should be more liberal with the truth

Added by Wil Cooley over 2 years ago. Updated about 2 years ago.

Status:RejectedStart date:06/08/2012
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:function
Target version:-
Keywords: Branch:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

str2bool’s idea of “true” should be “not false”. In other words, anything other than the recognized “false” strings should evaluate to true.

It would be nice to be able to use a selector as like a ternary operator:

define foo( $baz = undef ) {
  $real_baz = str2bool($baz) ? { true => $baz, false => $name }
}

This currently fails unless $baz is a recognized value for true or false, so one has to resort to this, which frankly makes my brain hurt:

$real_baz = $baz ? { undef => $name, default => $baz }

History

#1 Updated by Wil Cooley over 2 years ago

Github pull request #74 submitted: https://github.com/puppetlabs/puppetlabs-stdlib/pull/74

#2 Updated by Wil Cooley about 2 years ago

New pull request: https://github.com/puppetlabs/puppetlabs-stdlib/pull/103

#3 Updated by Jeff McCune about 2 years ago

  • Category set to function
  • Status changed from Unreviewed to Rejected

Will,

I’m going to mark this ticket as “rejected,” which I know seems a little harsh. I don’t mean for it to seem that way. It’s just that I think it’s important to preserve the intent of only allowing a limited set of inputs to produce the true and false results. All other inputs other than the limited set of expected inputs should produce an exception. The approach I suggest, and I’d like to re-open this ticket as a bug under, is to make str2bool properly handle undefined variables. It seems like it should already, and if it doesn’t then the real root cause of this issue is likely the bug that undefined values aren’t properly mapped to false.

Here are the comments copied from Github:

@wcooley

Reading through the description of the problem in #14908, it seems to me that the behavior of raising a ParseError when unexpected input is encountered is both intentional and a sound design decision. I’m concerned this change will have negative, unintended side effects. For example, the following input is exceptional behavior, which is good, but with the patch applies it will return true, which is surprising and bad:

$somevar = str2bool('FALSE')

To address the problem you’ve described, what do you think about changing the str2bool to handle undefined variables. The comment in the code indicates me str2bool should already handle undefined variables, meaning the following example should already behave as you expected it to. If it does not, this is probably a bug that we should fix:

define foo( $baz = undef ) {
  $real_baz = str2bool($baz) ? { true => $baz, false => $name }
}

#4 Updated by Wil Cooley about 2 years ago

For completeness, here are my responses from the Github pull request (should we have these kinds of discussion in the issue tracker or Github… or puppet-dev list?)…

First response:

Using 'undef' does indeed seem to cover my use case, although I am not totally confident that I understand why. I am a little unclear about whether undef, true and false are special keywords/identifiers in the Puppet DSL or if they are just bareword strings; it seems like they are special based on my testing. But the str2bool function raises a ParseError if the argument is not a String, so it much be implicitly casting the undef to either '' or "undef"? Hm... This is getting messy -- bareword & quoted undef is false, but bareword true is a ParseError: $ puppet apply --modulepath /tmp/fake-puppetdir/ -e 'notice(str2bool(true))' str2bool(): Requires either string to work with at line 1 on node xxx $ puppet apply --modulepath /tmp/fake-puppetdir/ -e 'notice(str2bool(undef))' notice: Scope(Class[main]): false notice: Finished catalog run in 0.02 seconds $ puppet apply --modulepath /tmp/fake-puppetdir/ -e 'notice(str2bool("undef"))' notice: Scope(Class[main]): false notice: Finished catalog run in 0.02 seconds $ puppet apply --modulepath /tmp/fake-puppetdir/ -e 'notice(str2bool("true"))' notice: Scope(Class[main]): true notice: Finished catalog run in 0.02 seconds My chief motivation is the feeling that the current behavior violates the Principle of Least Surprise -- the other dynamic or weakly-typed languages that I work with behave as I have described (or at a minimum do not introduce a 3rd potential outcome). At any rate, may I suggest at least merging commit 9b44596, as it add increases test coverage of the (currently) acceptable input? I can do the whole ticket-branch-pull-request thing if you'd like.

Second response:

@jeffmccune Sorry, had a data center power outage Wed; it's been a long 72 hours. What I said before about undef meeting my use-case is wrong. The define using undef example is exactly what currently does not work, but not because undef is not recognized as false, but because when $baz is set to anything but 'true', 'yes', etc. you get a ParseError!

Further thoughts: The “Functions” section in the new language reference manual clarifies what I was uncertain about wrt keyword undef, true and false and why str2bool(undef) works but str2bool(true) is a ParseError and as such is not really inconsistent.

Stepping back a bit, the whole trouble seems to stem from the fact that selectors cannot have arbitrary expressions, so I cannot coerce or cast a variable to a boolean with a hack like “!!$baz” or “$baz and $baz”. Assuming that restriction of selectors is for a design reason and not just an implementation artifact, then perhaps a function that explicitly converts to a boolean (as is done implicitly in the conditional of an “if” statement) would be called for?

Stepping back even further, what I really want is the ability to obviously and concisely select from several potentially undefined variables; “coalesce” is the name of the function in SQL… perhaps that would be best?

#5 Updated by Gary Larizza about 2 years ago

Will, is this what you’re looking for (re: coalesce) —> https://github.com/puppetlabs/puppetlabs-stdlib/pull/84

#6 Updated by Wil Cooley about 2 years ago

Gary, yeah, that works for me. Do you know if Jeff intends to merge it?

Not that it matters for my needs, but I do note that by using the existing ‘empty’ function in my implementation, I end up getting the ability to handle arrays and hashes for free. Seems like it might be more self-consistent for future maintenance to define it in terms of the existing notion of “empty” too.

#7 Updated by Jeff McCune about 2 years ago

Wil Cooley wrote:

Gary, yeah, that works for me. Do you know if Jeff intends to merge it?

I’ve merged in the pick() function. Sorry it took so long. =(

Not that it matters for my needs, but I do note that by using the existing ‘empty’ function in my implementation, I end up getting the ability to handle arrays and hashes for free. Seems like it might be more self-consistent for future maintenance to define it in terms of the existing notion of “empty” too.

I don’t think having one parser function call another parser function is bad in and of itself, but if we can avoid it it reduces tight coupling. Perhaps we could tease out an “emptiness” utility method and have both functions depend on the emptiness method at an API level?

-Jeff

Also available in: Atom PDF