Introduce <#unless>

[miguel-s]

Related to #340

[Malian]

@miguel-s I am torn between keeping the AST.If structure and adding a new AST.Unless structure

By using a new Unless struct will clarify the intent of the user and reduce cognitive overhead but I am totally fine with the If struct if we use the else attribute instead of reverting the condition.

I would also add tests in the if_test.exs to check the behaviour of #unless and not only the AST.

[miguel-s]

@Malian great suggestions, thanks!

I am torn between keeping the AST.If structure and adding a new AST.Unless structure

My reasons for going with AST.If:

1. Reusing existing code to speed up implementation, reduce our cognitive overhead and reduce maintenance costs
2. Inspired by Elixir's unless, which uses and inverts if

But I'm not married to the idea :)

I would also add tests in the if_test.exs to check the behaviour of #unless and not only the AST.

Definitely. Wasn't very happy with my solution, so wanted to check my approach with you before investing more time into this.

[lnr0626]

I think sticking with ast.if makes sense for the reasons @miguel-s listed.

I'd originally thought inverting the condition was the way to go (inspired by elixir's unless), but using the else block is a clever solution.

[msaraiva]

One downside of using AST.If is that since we're generating an if instead of an unless, any related error presented will be mentioning if, not unless, which might be confusing. For instance:

if true do
  1
else
  2 
else
  3
end

will raise:

(ArgumentError) invalid or duplicate keys for if, only "do" and an optional "else" are permitted

I'm not sure if we're already validating duplicated else when handling AST.If. If we do and the message is generic (just mentioning else), then I guess we're good. After all, it's an edge case.

Regarding unless...else, I've been working with Elixir for 6 years and I never saw that in any code. However, since Elixir's unless have it, we should consider supporting it too. I'm not the best person to judge that as I never used unless in my life and my head already hurts reading unless alone. Reading unless...else just short-circuits my brain :)

[Malian]

Regarding unless...else, I've been working with Elixir for 6 years and I never saw that in any code.

😱 I was glad to believe this did not exist!

However, since Elixir's unless have it, we should consider supporting it too. I'm not the best person to judge that as I never used unless in my life and my head already hurts reading unless alone. Reading unless...else just short-circuits my brain :)

I suggest not allow this at the first stage. If we receive feedback from users who want to add it, we will be able to consider supporting it too.

I'm not sure if we're already validating duplicated else when handling AST.If.

No we don't.

The following case is not validated too:

<#if {true}>
IF
<#else>
ELSE
<#elseif {true}>
ELSEIF
</#if>

[miguel-s]

I suggest not allow this at the first stage. If we receive feedback from users who want to add it, we will be able to consider supporting it too.

Agree. It might be even positive to not support it. We can always revisit this it if we get enough feedback.

One downside of using AST.If is that since we're generating an if instead of an unless, any related error presented will be mentioning if, not unless, which might be confusing.

I've looked and not been able to find any case like this. @Malian can you think of any place this could happen?

[Malian]

I've looked and not been able to find any case like this. @Malian can you think of any place this could happen?

With the current implementation I believe we cannot arrive in that specific invalid state. In the EEXEngine we have that piece of code that cannot have more than one if and one else option.

{:if, [generated: true], [condition, [do: if_buffer, else: else_buffer]]}

Nevertheless, a code that do not respect the if, if/else, if/elseif/elseif/..., if/elseif/.../elseif/else will fail at compile time with a useless message for the end-user:

** (KeyError) key :line not found in: %{}

[Malian]

@miguel-s It is easy to validate both cases (see my branch https://github.com/Malian/surface/tree/validate-wrong-if-elseif-else-constructs).

That being said, I am not sure if we should merge it as it make the compiler file more complex and the work that @lnr0626 is doing on #378 will help us to have a more maintainable code, so I suggest to wait until #378 is merged before tackling the validations.

WDYT?

[msaraiva]

I suggest to wait until #378 is merge before tackling the validations.

Agreed. 👍

[miguel-s]

I suggest to wait until #378 is merge before tackling the validations.

Sounds good to me.

@Malian @msaraiva what does it mean for this PR? Any changes needed, should I wait or ready to go?

[msaraiva]

@miguel-s this is good to go!