statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
154 stars 29 forks source link

Conditional tags evaluate 0 as true, which may be an unexpected corner case #103

Closed Frencil closed 7 years ago

Frencil commented 7 years ago

In #101 the {{#if foo}}{{/if}} conditional was changed such that the value (foo) is evaluated as true when 0. This solves the immediate problem that arises when using the conditional tag for determining if a field is populated with any value but may break expected behavior should a value of 0 need to be evaluated as false.

A potential solution would be to expand the conditional tag to optionally support an operator and comparison value, allowing for more explicitly defined behavior.

pjvandehaar commented 7 years ago

The functionality I want is foo || foo === 0.

Syntax options:

pjvandehaar commented 7 years ago

Currently, we parse templates with

/\{\{(?:(#if )?([A-Za-z0-9_:\|]+)|(\/if))\}\}/;

but the new regex will be:

var field = '[A-Za-z0-9:|_]+';
var thing = '(?:' + field + '|"(?:[^"]|\\\\")*"|[0-9]+)';
var op = '(?:===|!==|<|>)';
var bool = '\\s*(?:!\\s*)*'+thing+'\\s*';
var comp = '\\s*'+thing+'\\s*'+op+'\\s*'+thing+'\\s*';
var expr = '(?:' + bool + '|' + comp + ')';
var disjoint = '(?:' + expr + '(?:\\|\\|' + expr + ')*)';
var ifregex = '{{#if ' + disjoint + '}}';
var varregex = '{{' + field + '}}';
var endifregex = '{{/if}}';
var regex = new RegExp(ifregex +  '|' + varregex + '|' + endifregex);

which evaluates to

/{{#if (?:(?:\s*(?:!\s*)*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*|\s*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*(?:===|!==|<|>)\s*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*)(?:\|\|(?:\s*(?:!\s*)*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*|\s*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*(?:===|!==|<|>)\s*(?:[A-Za-z0-9:|_]+|"(?:[^"]|\\")*"|[0-9]+)\s*))*)}}|{{[A-Za-z0-9:|_]+}}|{{\/if}}/
Frencil commented 7 years ago

I'd propose a third option; something that lets basic conditionals use expected javascript evaluations of truthiness but allow for an explicit "is defined" check (maybe something along the lines of {{#ifdefined {{var}}}}...{{/ifdefined}}). In your use case here it seems you only care about showing something if it's defined and don't really care about its value if it is, so maybe expanding out the conditional logic to allow for operators and compound conditionals is an overkill approach to a much simpler problem.

pjvandehaar commented 7 years ago

But, in my case, I want empty strings to be falsy.

Frencil commented 7 years ago

{{ifnotempty {{var}}}}...{{/ifnotempty}}?

Certainly gargantuan regexes are an indication that we're not heading in the right direction. A more explicit tag that is purpose-built and can be clearly documented with clean syntax is going to play much nicer for clarity and long term maintainability.

pjvandehaar commented 7 years ago

(rows with disagreements are capitalized)

Boolean() not empty what most users want
0 FALSE TRUE TRUE
1, inf true true true
"" false false false
"x" true true true
[] TRUE FALSE ??
[1] true true ??
{} TRUE FALSE ??
{x:1} true true ??
true true true true
false FALSE TRUE ??
null false false false
NaN FALSE TRUE ??
undefined false false false
  1. I like the term not empty, good idea.

  2. I see two types of usage of guards:

    • automated use, where I need a single default test that minimizes both junk printed and information hidden
    • explicit use, where users will want a variety of different tests depending on the field
      • eg, false should be shown in is_imputed but hidden for isldrefvar.
      • eg, arrays and objects will always need special full-line formatters, so they might as well always test true and let the formatter return "" if desired.

    If we'd only need a few tests, it might make sense to make each one a {{#iffoo var}}, but with lots (and a need for customization), transformations make more sense.

  3. Should we roll-back my 0-is-truthy PR and instead add and document

    LocusZoom.TransformationFunctions.add("is_not_empty", function(x) {
        if (typeof x === "undefined") return false;
        if (typeof x === "number") return true; // note: typeof NaN === "number", despite the name
        if (typeof x === "boolean") return true;
        if (typeof x === "object" || typeof x === "string") { handles array, object, null, string
            for (var k in x) if (x.hasOwnProperty(k)) return true;
            return false;
        }
        console.error("what is " + x.toString() + "?"); return true;
    });

    ?

Frencil commented 7 years ago

In response to the table my assumptions about what "not empty" means would differ with yours for [] and {} and could really see NaN going either way. The term is clearly too ambiguous, so isn't a viable solution to this problem.

At this point I'd be wary of overcomplicating this issue. We've solved the immediate problem for PheWeb's use case, and the behavior is well documented. Let's let it be until an actual use case for 0-falsey behavior presents itself.