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 @or helper and tests #100

Closed
wants to merge 1 commit into from
Closed

Add @or helper and tests #100

wants to merge 1 commit into from

Conversation

rragan
Copy link
Contributor

@rragan rragan commented Nov 10, 2014

As a related item to the desire to deprecate the if helper, I've developed an or helper since there is no good way to handle the or capabilities provided with the if helper. The "and" operation can be done by nesting eq/ne/... helpers so there is an alternative for it.

@sethkinast
Copy link
Contributor

Before we get into a code review-- the syntax for this feels a little weird to me. Self-closing conditions, true and false blocks, and the "or" is the parent-level tag.

I think the concept of "or" is extremely necessary. I worked on some hideous code last week that should have been just a few lines but for the lack of "or".

That said, I am definitely open to looking at core-grammar changes or {@eq} et al. helper changes to allow them to support multiple conditions in a way that feels a little more intuitive.

@rragan
Copy link
Contributor Author

rragan commented Nov 10, 2014

Yeh it is a tad weird. The main block should only contain conditions. Having a body does not make much sense for this sort of usage but it did make me have to suppress any other chunk output in the main block as that would also be irrelevant. Any other helpers that output to chunk in the main block would also have output suppressed.

The block name does not bother me so much. We have :else already and the meaning of this would be clear to reader at a glance.

Opening up new syntax avenues was not on my radar. Piling the conditions on to the parent or tag would likely get messy fast and be hard to read unless the compiler could be made to handle some new syntax.

@rragan
Copy link
Contributor Author

rragan commented Nov 23, 2014

Here is another slightly different syntax I have working. It is more infix visually so might be less confusing. Also it allows short circuit evaluation (assuming the throw is not mucking up anything after I catch it).

{@test}
    {@eq key=2 value=3 /}
    {@or /}
    {@ne key=3 value=3 /}
    {@or /}
    {@eq key=3 value=5 /}
    {:true}true 
{/test}

@sethkinast
Copy link
Contributor

Hey Richard,

We had a meeting today to talk about or and the concept of multiple values in general. What do you think about a syntax like this?

{! foo is 1 or 2 or 3 !}
{@select key=foo}
  {@eq value="1"/}
  {@eq value="2"/}
  {@eq value="3"/}
  {@any}Foo is 1, 2, or 3{/any}
  {@default}Foo is not 1, 2, or 3{/default}
{/select}

This is a little more flexible than true or false blocks:

{! Output something specific as well !}
{@select key=foo}
  {@any}Congratulations! You got: {/any}
  {@eq value="1"}First Place{/eq}
  {@eq value="2"}Second Place{/eq}
  {@eq value="3"}Third Place{/eq}
  {@default}Better luck next time!{/default}
{/select}
{! If any truth test passes, all {@any} blocks will be evaluated !}
{@select key=foo}
  {@any}<span class="place">{/any}
  {@eq value=1}First Place{/eq}
  {@eq value=2}Second Place{/eq}
  {@eq value=3}Third Place{/eq}
  {@any}</span>{/any}
{/select}

The functionality this does not include is the ability to compare multiple keys (e.g. a=1 || b=2 || c=3), but our thought is that that's too much logic for a template to contain directly (and that logic should be part of a context helper or delivered by the backend).

We also discussed expanding the Dust syntax to allow arrays:

{@or key=foo values=[a,"bar",3]}...{/or}

And the two versions of {@or} you provided. This seemed like a good balance of existing syntax (not using {:true} blocks, not modifying the grammar to understand arrays) with new functionality.

@rragan
Copy link
Contributor Author

rragan commented Dec 10, 2014

First thoughts. @select is, at heart, more like an AND operation since multiple tests can be true and produce output (easier to consider with gt rather than eq). Quite how to implement the multiple any's behavior might prove to be challenging at first thought. Seems like some body capture with deferred emits might be needed.

Note that you already have the ability to have multiple keys since eq/ne/... take both key and value and the local key overrides whatever was on the select. If you wanted to go a tad further you could make omitting the key on the select an option and make the eq/ne/... that failed to then provide a key be the error. This get much closer to my initial or version with any at the end replacing the true block.

This does not allow short circuit behavior which my second form did (although I'm not 100% sure that would work in all cases -- throw/try/catch can be tricky)

Won't someone soon ask for an all helper?

@sethkinast
Copy link
Contributor

I admit I haven't read the {@select} code in a bit. I'll do so now. But the docs say:

Each test condition is executed in order. If a condition is true, its body is output and all subsequent condtions are skipped.

Doesn't {@select} short-circuit today? Or are our docs wrong (oops)?

@rragan
Copy link
Contributor Author

rragan commented Dec 10, 2014

Ah yes, it does -- I too misrembered. The current code does not really short circuit. It just sets the isResolved flag which returns false to every subsequent eq/ne/...

@sethkinast
Copy link
Contributor

OK, I was worried that the fundamental assumption is wrong.

//  supports only one of the blocks in the select to be selected
if (context.current().isResolved) {
  filterOp = function() { return false; };
}

So I think short-circuiting should already be a thing (and was the intent).

We actually brought up an {@ALL} helper! But without considering the fact that you could override the keys inside the block. However, I'm not sure that's "officially" supported-- the docs don't even mention it. (I didn't realize you could do it.)

@sethkinast
Copy link
Contributor

OK, gotcha-- it's sort of fake-y short-circuiting since it just no-ops.

@sethkinast
Copy link
Contributor

I think with the any blocks, you create an async chunk that resolves at a later time. I haven't written any code yet to think about how it might actually be done, but it does seem kind of useful especially in the third example where you wrap results in some HTML.

@rragan
Copy link
Contributor Author

rragan commented Dec 10, 2014

The fact that a key param overrides the key from select is an artifact of the code. Undocumented, except inasmuch that key is allowed anyway on eq, etc. The select docs certainly do suggest that they key has to be on the select and not the eq.

@sethkinast
Copy link
Contributor

Yeah, I see the problem in the code. We test if it's on the helper before
we see if it's provided by the select, when they should be switched. I'll
log a defect for that; it's a backwards-incompatible change.

On Tue, Dec 9, 2014 at 5:09 PM, Richard Ragan notifications@github.com
wrote:

The fact that a key param overrides the key from select is an artifact of
the code. Undocumented, except inasmuch that key is allowed anyway on eq,
etc. The select docs certainly do suggest that they key has to be on the
select and not the eq.


Reply to this email directly or view it on GitHub
#100 (comment)
.

Seth Kinast
http://sethkinast.com/

@rragan
Copy link
Contributor Author

rragan commented Dec 10, 2014

Awww, Could be useful for multiple test and even for select but we would want to legitimate it in the docs.

@sethkinast
Copy link
Contributor

I added a comment instead of adding an issue for now-- I thought about the case where you have a nested truth test that you might want to put your own key on. What do you think of #107? I think it's more Dust-like than specially-named blocks.

@sethkinast sethkinast closed this Mar 4, 2015
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

2 participants