Skip to content

ELSE statement for FOREACHes that have no elements #55

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

pplu
Copy link
Contributor

@pplu pplu commented Oct 28, 2015

Hi,

I've updated a patch to TT that I developed with @miquelruiz that we sent some time ago.

It implements an ELSE block for FOREACH statements that have no elements in them. Everything is OK, except for one test in t/vars.t that I don't understand why is failing. It looks like it's failing to call the function "yankee" to get the values

@pplu
Copy link
Contributor Author

pplu commented Oct 28, 2015

Hi,

I found the bug, and squatted it :)

@pplu
Copy link
Contributor Author

pplu commented Oct 19, 2017

Hi,

Any thoughts on this featue? It is inspired by other templating systems (like PHPs smarty templates: https://www.smarty.net/docs/en/language.function.foreach.tpl)

@toddr
Copy link
Collaborator

toddr commented Oct 6, 2018

@pplu My initial comment would be that I see the value of this but the style is very unperlish in a templating language that does perl. At the moment, I can't think of any other TT feature that isn't also doable in perl.

Todd

@pplu
Copy link
Contributor Author

pplu commented Oct 8, 2018

I always saw TT as a language of it's own (not mapping 1 to 1 Perl). It compiles to Perl in the background, but it's syntax doesn't have much to do with Perl (IMHO). Seeing this feature in other templating languages I thought that it's nice that TT keeps up with others.

The feature is a shorthand for a common construct: Iterate over a list if there are elements, else do something else. The "do something else if the list is empty" part is cumbersome in current TT, making you write nested code.

[% IF (list.count == 0) %]
  The list is empty
[% ELSE %]
  [% FOREACH el = list %]

  [% END %]
[% END %]

The same construct with the ELSE is more compact, and nicer to write when you've written the FOREACH, and you realize "now I have to handle the empty list case".

[% FOREACH el = list %]

[% ELSE %]
  The list is empty
[% END %]

@atoomic
Copy link
Collaborator

atoomic commented Oct 8, 2018

This looks a pretty low cost feature, that I could see useful in more than one case.
This would be good to provide some kind of minimal documentation about it, in order to have people start using it, otherwise its value would be very limited.

👍

@atoomic
Copy link
Collaborator

atoomic commented Oct 8, 2018

@pplu also note that currently the branch is conflicting with master, I'm perfectly aware that this was expected as this feature was pending for a few years...

Any chance you could resubmit an updated merge request after rebasing on master ?
Doing this would also enable travis smoke on your work.

thanks in advance

@toddr
Copy link
Collaborator

toddr commented Oct 8, 2018

How would this work with pre-compiled templates?

(note: conflict in lib/Template/Grammar.pm was resolved executing ./yc in parser dir)
@pplu
Copy link
Contributor Author

pplu commented Oct 9, 2018

@toddr : from what I understood at the time: TT always converts the templates to Perl, and then runs the resulting Perl code. What TT does when you use precompiled templates (COMPILE_DIR option), the Perl version of the templates is cached to disk. Since this feature changes the Perl code generated for the FOREACH statements, it should play along well with the caching behavior.

@atoomic : I've merged the current master. The conflicts resolution in Grammar.pm was just a question of executing the yc script in the parse directory. I'll generate the docu.

@pplu
Copy link
Contributor Author

pplu commented Oct 9, 2018

@atoomic I've documented this in Template/Manual/Directives.pm, together with other options and behaviors of the FOREACH directive.

@atoomic atoomic force-pushed the master branch 5 times, most recently from 0c2da20 to e0d5f00 Compare October 11, 2018 22:46
@pplu
Copy link
Contributor Author

pplu commented Dec 28, 2018

@atoomic poke

@@ -429,10 +429,11 @@ sub if {
#------------------------------------------------------------------------

sub foreach {
my ($self, $target, $list, $args, $block, $label) = @_;
my ($self, $target, $list, $args, $block, $else, $label) = @_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

when I read this, I've the feeling that this going to break previous behaviors where foreach was called with a label, as the label is going to be shift in else statement.

One suggestion would be to move the else as last argument of this function and use an extra helper in the parser that would wrap this function

@@ -165,13 +165,17 @@ case: CASE term ';' block
;

loop: FOR loopvar ';' { $_[0]->enter_block('FOR') }
block END { $factory->foreach(@{$_[2]}, $_[5], $_[0]->leave_block) }
block loopelse END { $factory->foreach(@{$_[2]}, $_[5], @{$_[6]}, $_[0]->leave_block) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

something like $factory->foreach_else that will call the foreach helper with the correct args and the else statement in last position so this would be backward compatible

@atoomic
Copy link
Collaborator

atoomic commented Jan 7, 2019

thanks @pplu for the patch, sorry for my late answer but after reading the patch I've the feeling that this is going to break previous behaviors, it would be safer to use a dedicated helper for this view comments on the Pull Request.

if I'm right, this also shows that the testsuite for 'foreach' statement needs some improvement and the label are not tested

could you provide an updated patch
thanks

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.

3 participants