Forum OpenACS Development: Is there a right way to deal with booleans?

While searching for a tiny bug in bboard I stumbled over an annoying source for confusion with boolean values in tcl vars:

In bboard/www/forum-view.adp the link to create new categories is always displayed, although it should be hidden when the user does not have the permission to add categories. A look in the adp file explains why: the boolean tcl variable category_create_p is tested assuming that it contains either 0 or 1, while it actually contains t or f.

The standard format for boolean values returned from sql is t/f, while (most? all?) boolean tcl functions from the acs api
(e.g. ad_permission_p) return 0/1. As long as the code is in the same file as the queries it is easy to see which variables contain which kind of value pairs, but when writing the .adp there is no more indication for that, and one has to constantly swap between the files to look it up.

Is there some general rule that should be applied here? Maybe modify the queries to return 0/1?

(Side note: what further confused me is that tcl allows for t/f inside an if { } conditional, but not as part of an expression; e.g. [expr ! t] throws an error, while [expr ! 1] returns 0)

Collapse
Posted by David Walker on
Probably the best way to handle booleans is with the [string is true $bool]
or [string is false $bool] command. (check the docs.  you might want a -strict
in there depending on your needs)

This command will accept the words true or false, the letters t or f, or the
numbers 1 or 0.  The command always returns 1 or 0.

Collapse
Posted by Don Baccus on
Hey, did that exist in Tcl 7.6?  I should go reread the Tcl 8 manual ... or maybe it was there all the time and none of us ever noticed.

Yes, David's suggestion's excellent, especially when dealing with database queries (there are some that return 0 or 1, using count, in total inconsistency with the normal t/f).

For efficiency reasons just doing the boolean 0/1 test within Tcl code is something we want to maintain in some contexts.  For instance the boolean parameters supported by ad_proc are guaranteed to be 0/1 and I think that's true of boolean ad_page_contract parameters, too ...

Collapse
Posted by David Walker on
It is new for tcl 8.  I have an advantage since I only ever learned tcl 8.  I had
to look up the old docs to see if it was there.
Collapse
Posted by Stephen . on
The templating system is being used incorrectly. Try:

<if @category_create_p@ true>
...
Collapse
Posted by Don Baccus on
I missed that this was an .adp page, not .tcl page.  Sheesh.

Stephen - does the template code work for "t/f" and "0/1" both?  (if not we should think about changing the engine ...)

Collapse
Posted by Stephen . on
Yes, the if tag uses template::util_is_true, which is used by alot of
the rest of the template system code. It checks for the negative
conditions 0, f, false, n, no, off, and the empty string, everything
else being true.
Collapse
Posted by Tilmann Singer on
<code>&lt;if @bool_p@ true&gt;</code> is quite a useful function, thanks! This should definitely go as an example for usage of &lt;if ... &gt; into the templating documentation. The original toolkit developers must have missed this too - grepping for "@ true" and "@ false" yields no results except in the acs-templating demos.
<p>
Also I could not find one place where template::util::is_true is used in the templating code, except in tcl/tag-procs.tcl where the if tag is being defined.
<p>
Looking further in the bboard templates, there are lots of this: <code>&lt;if @headings_p@ not nil and @headings_p@ ne "f"&gt;</code>. Wouldn't it be nice being able to replace this with <code>&lt;if @headings_p@ true&gt;</code>? Or even nicer: <code>&lt;if @headings_p@&gt;</code>? (Which one would you rather want to explain to a graphics designer?) This would require to alter the proc that deals with if conditionals a little bit so that it interprets undefined variable names as false.
<p>
It would make sense because in most situations booleans in .adp's are used to test if something optional should be included in the page. If it was not set, well then just don't include the optional stuff. Especially in included .adp's that are used from different contexts it's likely to encounter variables that are not defined.
<p>
Ah well, propably something for the post-release wishlist ...
Collapse
Posted by Stephen . on
You're right, is_true is used hardly anywhere! I've just been using it myself...

Shouldn't nil be used if variable_name nil not if @variable@ nil?

According to this: /doc/acs-templating/demo/show.tcl?file=if.adp: no. It uses <if @variable@ nil>
Collapse
Posted by Stephen . on
Yeah, but the code that handles nil treats it's argument as the name of a variable. If the name of the variable exists, then the value of the variable is checked for the empty string.

The docs and that bboard code pass an argument as @value@, which will be interpolated just like a Tcl $variable (right?). I think the docs and that bboard code must be wrong.

Both versions: <if foo nil> and <if @foo@ nil> work. There is a regsub in the templating code (tag-procs.tcl) that converts "@foo@" to "foo" if necessary before evaluating it with info exists.

The <if foo nil> variant makes more sense to those familiar to tcl, but <if @foo@ nil> is somehow more coherent with the special syntax that's used in the templates (simply everything within @'s is a variable), and certainly is easier to explain to people not familiar with tcl.

Which of those version is the 'right' one is hard to answer without docs, and besides the demo and the examples in that page: /doc/acs-templating/tagref/if.html I did not find any hints on this. I'd rather vote for the <if @foo@ nil> variant though, because of the easier-to-explain-to-non-tclers advantage.

Collapse
Posted by Don Baccus on
Yeah, I think the @foo@ form is nicer, too...

Boy, we should weed out all those ne 0 etc expressions.

Tilmann, if you have time and want to at least tackle the documentation I'm sure Roberto would accept changes from you.

Collapse
Posted by Stephen . on
You're right, makes sense. (It's not Tcl).
A grep for _p@ (ne|eq) (0|1|t|f) matches 250 lines in more then 20 different packages - I'll start my find-and-replace engine and send one patch for all of those to you Don, alright?

But before that I'd like to ask if the change to the templating code mentioned above would be considered useful. I suggest to change the behaviour of <if> so that a non-defined variable in a boolean comparison will be interpreted as false instead of throwing an error as it would do currently, and also define true as default operator when only one variable and no operator is given.

Then template writers could say <if @bool_p@> instead of <if @bool_p@ not nil and @bool_p@ true>, which does not only look better and saves some typing, but is also a lot easier to explain to not-so-technical template writers.

Do you think this is useful and harmless enough to be added before the beta release?

Collapse
Posted by Ken Kennedy on
One thing to note is that considering <if @bool_p@> as false automatically (when the variable is not defined) opens you up to misspelling errors of the type <if @boolp@>, or <if @bol_p@> always silently returning false...
Collapse
Posted by Don Baccus on
Also in other contexts undefined datasources throw an error so ... personally I vote "no" for the "not nil".  The default true case is a reasonable shorthand though.

What do others think?

Collapse
Posted by Don Baccus on
Oops ... my post is confused, sorry :)

1. shorthand for "true" - no problem IMO

2. undefined returning "false" ... I don't like that, it's inconsistent with the rest of the templating system and, as Ken says, makes you vulnerable to simple typing errors.

3. dropping "not nil"?  Is it really needed?  All I should need to say is "if @foo@ true" and if that isn't working correctly then something's broken IMO.

Collapse
Posted by Ken Kennedy on
I like/vote for the default true, and "if @foo@ true" syntax. Simple is good, but getting TOO simple by making too many assumptions, and we might as well use perl...*grin* (Not that I don't like perl...I just can't READ it half the time. Even what I just wrote!)
Here is a patch for the <if @bool_p@> shorthand: Patch 124 - it just appends "true" to the condition if there is no other operator and does not change the way undefined variables are handled.

Well ok, I am convinced that the nil==false wasn't such a great idea ...

Collapse
Posted by Don Baccus on
Thanks ... I have a backlog of patches, including this one, that I need to work through and apply.  I'll try to get them in this week.