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 in the `maybe` and `with` control statements #9

Closed
cdepillabout opened this issue Jan 2, 2017 · 16 comments
Closed

Add in the `maybe` and `with` control statements #9

cdepillabout opened this issue Jan 2, 2017 · 16 comments

Comments

@cdepillabout
Copy link
Contributor

@cdepillabout cdepillabout commented Jan 2, 2017

Would it be possible to add in the maybe and with control statements?

http://www.yesodweb.com/book/shakespearean-templates#shakespearean-templates_maybe

http://www.yesodweb.com/book/shakespearean-templates#shakespearean-templates_with

It would also be nice to have a case statement, but that can be easily worked around with if.

http://www.yesodweb.com/book/shakespearean-templates#shakespearean-templates_case

These additional statements would make my current work (blueimpact/kucipong#77) on kucipong much easier.

I can try implementing these if you'd like!

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 2, 2017

@arowM I added a PR #10 to clean up the Text.Heterocephalus.Parse module.

If you agree we should add maybe and with control statements, I can try to add it next week in a separate PR.

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 3, 2017

@cdepillabout I agree with the opinion that these additional control statements makes some code easier.
But basically, I believe heterocephalus should not provide convenient control statements.

Heterocephalus is supposed to be used with another template engine. Hence, heterocephalus only has a role to inject template variables determined by Haskell back-end server.
Providing many convenient control statements makes us a bit happy, but is it important for this role? I believe that it is more important to keep heterocephalus simple to learn or read.

About case and maybe statements, it is not necessary to realize the heterocephalus role I mentioned previously.
As well as case statement you referred, maybe is not necessary as your link says.

This could technically be dealt with using if, isJust and fromJust

About with control statements, we should bind the variable name in Haskell code, because using with in a template file makes View fat improperly. I think using with is a kind of bad know how.

I propose that it is much important to provide elseif statement rather than providing above control statements.

Of course, if you tell me actual use cases that can be enough good to require above disadvantages by providing these control statements, I'll rethink about it.

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 7, 2017

I agree that not having maybe, case and with control statements makes hetercephalus much simpler. However, it causes the templates to be pretty complicated.

Here's some examples of templates I'm writing for blueimpact/kucipong#86, and how they could be more simple with maybe, case, and with control statements:

Example 1

Here is an excerpt from the bottom of frontend/src/pug/storeUser_store_coupon.pug.

    | %{ if null couponEntities }
    |   No Coupons
    | %{ else }
    |   %{ forall couponEntity <- couponEntities }
    include _couponSimple
    |   %{ endforall }
    | %{ endif }

And then here is the frontend/src/pug/_couponSimple.pug file, which is being included above:

.couponSimple
  a.card(href="/store/coupon/\#{ fromSqlKey (entityKey couponEntity) }")
    .annotatedImageGroup
      img.annotatedImageGroup-image(src=require("../img/dummy.jpg") alt="\#{ couponTitle (entityVal couponEntity) }")
      span.annotatedImageGroup-annotation 画像は一例です
    .card_body
      .couponSimple_abstraction
        h2.couponSimple_abstraction-store \#{ couponTitle (entityVal couponEntity) }
        .couponSimple_abstraction-summary

          | %{ if couponTypeIs couponEntity CouponTypeDiscount }
          |     %{ if isJust (couponDiscountPercent (entityVal couponEntity)) }
          span.couponSimple_abstraction-summary-sub \#{ fromMaybe 0 (couponDiscountPercent (entityVal couponEntity)) }% OFF
          |     %{ endif }
          |     %{ if isJust (couponDiscountMinimumPrice (entityVal couponEntity)) }
          span.couponSimple_abstraction-summary-main \#{ fromMaybe 0 (couponDiscountMinimumPrice (entityVal couponEntity)) }円
          |     %{ endif }
          | %{ endif }

          | %{ if couponTypeIs couponEntity CouponTypeGift }
          | ...
          | %{ endif }

          | %{ if couponTypeIs couponEntity CouponTypeSet }
          | ...
          | %{ endif }

          | %{ if couponTypeIs couponEntity CouponTypeOther }
          | ...
          | %{ endif }

There are three annoying things about this:

  1. Because there is no with control statement, it is necessary to write entityVal before every couponEntity in _couponSimple.pug.
  2. Because there is no case control statement, it is necessary to use multiple if control statements when checking the coupon type of couponEntity. This isn't too bad (and it would look even nicer with the elseif control statement), but it would be typesafe if we had a case statement. The compiler would show a warning if we forgot to include a coupon type.
  3. Because there is no maybe control statement, it is necessary to use isJust and fromMaybe. This also isn't too bad, but it would be less code with a maybe control statement.

Here's how I would rewrite it with maybe, case, and with control statements.

frontend/src/pug/storeUser_store_coupon.pug:

    | %{ if null couponEntities }
    |   No Coupons
    | %{ else }
    |   %{ forall couponEntity <- couponEntities }
    |     %{ with coupon <- entityVal couponEntity }
    |     %{ with couponKey <- entityKey couponEntity }
    include _couponSimple
    |     %{ endwith }
    |     %{ endwith }
    |   %{ endforall }
    | %{ endif }

frontend/src/pug/_couponSimple.pug:

.couponSimple
  a.card(href="/store/coupon/\#{ fromSqlKey couponKey }")
    .annotatedImageGroup
      img.annotatedImageGroup-image(src=require("../img/dummy.jpg") alt="\#{ couponTitle coupon }")
      span.annotatedImageGroup-annotation 画像は一例です
    .card_body
      .couponSimple_abstraction
        h2.couponSimple_abstraction-store \#{ couponTitle coupon }
        .couponSimple_abstraction-summary

          | %{ case couponType coupon }

          | %{ of CouponTypeDiscount }
          |     %{ maybe percent <- couponDescountPercent coupon }
          span.couponSimple_abstraction-summary-sub \#{ percent }% OFF
          |     %{ endmaybe }
          |     %{ maybe minPrice <- couponDiscountMinimumPrice coupon }
          span.couponSimple_abstraction-summary-main \#{ minPrice }円
          |     %{ endmaybe }

          | %{ of CouponTypeGift }
          | ...

          | %{ of CouponTypeSet }
          | ...

          | %{ of CouponTypeOther }
          | ...

          | %{ endcase}

Example 2

Here's an example from frontend/src/pug/storeUser_store_coupon_id.pug:

    | %{if isJust maybeCouponEntity}
    .store
      .store_editBtn
        a.btn.outerBtn(href="/store/coupon/\#{fromSqlKey (entityKey (fromJustEx maybeCouponEntity))}/edit") クーポン情報編集
    include _coupon_id
    | %{else}
    | No coupon info.
    | %{endif}

And here is part of frontend/src/pug/_coupon_id.pug:

.coupon
  .card
    .card_header
      h2.card_header_text %{if isJust maybeStoreEntity}\#{ storeName entityVal) maybeStoreEntity}%{else}(no title)%{endif}
      .card_header_icon
        .icon.icon-fav.checkableIcon
          input#js-fav-3.checkableIcon-check.js-fav(type="checkbox" data-coupon-id="\#{fromSqlKey (entityKey (fromJustEx maybeCouponEntity))}")
          label.checkableIcon-icon(for="js-fav-3")

    .annotatedImageGroup
      img.annotatedImageGroup-image(src=require("../img/dummy.jpg") alt="七輪焼き肉・安安")
      span.annotatedImageGroup-annotation 画像は一例です
    .card_body
      .card_body_title
        | \#{couponTitle (entityVal (fromJustEx maybeCouponEntity))}

      .card_body_summary.highlightedBlock
        | %{ if couponTypeIs (fromJustEx maybeCouponEntity) CouponTypeDiscount }
        |     %{ if isJust (couponDiscountPercent (entityVal (fromJust maybeCouponEntity))) }
        span.highlightedBlock-sub \#{ fromMaybe 0 (couponDiscountPercent (entityVal (fromJust maybeCouponEntity))) }% OFF
        |     %{ endif }
        |     %{ if isJust (couponDiscountMinimumPrice (entityVal (fromJust maybeCouponEntity))) }
        span.highlightedBlock-main \#{ fromMaybe 0 (couponDiscountMinimumPrice (entityVal (fromJust maybeCouponEntity))) }円
        |     %{ endif }
        | %{ endif }

This code would also be slightly easier to write with case, maybe, and with statements (for the same reasons as above).

Here is how I would rewrite it:

frontend/src/pug/storeUser_store_coupon_id.pug:

    | %{ maybe couponEntity <- maybeCouponEntity }
    |     %{ with couponKey <- entityKey couponEntity }
    |     %{ with coupon <- entityVal couponEntity }
    .store
      .store_editBtn
        a.btn.outerBtn(href="/store/coupon/\#{fromSqlKey couponKey}/edit") クーポン情報編集
    include _coupon_id
    |     %{ endwith }
    |     %{ endwith }
    | %{ nothing }
    | No coupon info.
    | %{ endmaybe }

frontend/src/pug/_coupon_id.pug:

.coupon
  .card
    .card_header
      h2.card_header_text \#{maybe "(no store title)" (storeName . entityVal) maybeStoreEntity}
      .card_header_icon
        .icon.icon-fav.checkableIcon
          input#js-fav-3.checkableIcon-check.js-fav(type="checkbox" data-coupon-id="\#{fromSqlKey couponKey}")
          label.checkableIcon-icon(for="js-fav-3")

    .annotatedImageGroup
      img.annotatedImageGroup-image(src=require("../img/dummy.jpg") alt="七輪焼き肉・安安")
      span.annotatedImageGroup-annotation 画像は一例です
    .card_body
      .card_body_title
        | \#{couponTitle coupon}

      .card_body_summary.highlightedBlock
        | %{ case couponType coupon }

        | %{ of CouponTypeDiscount }
        |     %{ maybe percent <- couponDiscountPercent coupon }
        span.highlightedBlock-sub \#{percent}% OFF
        |     %{ endmaybe }
        |     %{ maybe minPrice <- couponDiscountMinimumPrice coupon }
        span.highlightedBlock-main \#{minPrice}円
        |     %{ endmaybe }

        | %{ of CouponTypeGift }
        | ...

        | %{ of CouponTypeSet }
        | ...

        | %{ of CouponTypeOther }
        | ...

        | %{ endcase }

I think having maybe, with, and case statements makes the templates easier to read since you don't constantly have to unwrap things with fromJust, entityVal, isJust, etc.

But maybe there is some way of writing the above templates by making use of pugs features so it is easier to read/write?

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 8, 2017

Thanks for real examples, @cdepillabout.
Your observations are enough significant to change my thoughts.
I have now following opinions about heterocephalus control statements.

  1. Heterocephalus should provide case statement
  2. Heterocephalus should provide elseif statement
  3. Heterocephalus still need not to provide maybe statement
  4. Heterocephalus still must not provide with statement

1. Heterocephalus should provide case statement

it would be typesafe if we had a case statement

This makes sense! I've noticed that in Haskell case is not just a syntax sugar but a core functionality to make codes type safe.

2. Heterocephalus should provide elseif statement

This is also important to make nests shallow.
It has no problem to accept this statement because the only reason heterocephalus does not have is I did not have enough time to do it.

3. Heterocephalus still need not to provide maybe statement

The example you pointed is not enough attractive for me to increase number of control statements heterocephalus provides.
Furthermore, case statement can be an alternative to maybe statement as follows.

An example with maybe statement:

| %{ maybe a <- xxx }
.just(value="\#{a}")
| %{ nothing }
.nothing
| %{ endmaybe }

An equivalent one with case statement:

| %{ case xxx }
| %{ of (Just a) }
.just(value="\#{a}")
| %{ of _ }
.nothing
| %{ endcase }

4. Heterocephalus still must not provide with statement

I feel that your example is still a sort of bad know how to make View (= template file) fat.

About frontend/src/pug/storeUser_store_coupon_id.pug, you should declare maybeCouponKey and maybeCoupon in Haskell program.

About frontend/src/pug/storeUser_store_coupon.pug, you can rewrite without with as follows.
(It assumes that couponEntityPairs is declared in Haskell program.)

| %{ if null couponEntities }
|   No Coupons
| %{ else }
|   %{ forall (coupon, couponKey) <- couponEntityPairs }
include _couponSimple
|   %{ endforall }
| %{ endif }

What do you think, @cdepillabout?
Again, thanks for realistic examples!

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 9, 2017

Okay, that makes sense.

Using the case control statement to handle Maybe values is a good idea.

Here's an example from above using the maybe and with control statements.

    | %{ maybe couponEntity <- maybeCouponEntity }
    |     %{ with couponKey <- entityKey couponEntity }
    |     %{ with coupon <- entityVal couponEntity }
    .store
      .store_editBtn
        a.btn.outerBtn(href="/store/coupon/\#{fromSqlKey couponKey}/edit") クーポン情報編集
    include _coupon_id
    |     %{ endwith }
    |     %{ endwith }
    | %{ nothing }
    | No coupon info.
    | %{ endmaybe }

It could be rewritten like this using just the case control statement:

    | %{ case maybeCouponStuff }
    | %{ of Just (couponKey, coupon) }
    .store
      .store_editBtn
        a.btn.outerBtn(href="/store/coupon/\#{fromSqlKey couponKey}/edit") クーポン情報編集
    include _coupon_id
    | %{ of Nothing }
    | No coupon info.
    | %{ endcase }

However, here's one example from one of the original documents (using just the if control statement):

| %{ if isJust (couponDiscountPercent coupon) }
span.couponSimple_abstraction-summary-sub \#{ fromJust (couponDiscountPercent coupon) }% OFF
| %{ endif }

Using the maybe control statement, it could be rewritten like this:

| %{ maybe percent <- couponDiscountPercent coupon }
span.couponSimple_abstraction-summary-sub \#{percent}% OFF
| %{ endmaybe }

However, using the case control statement would be a little more verbose:

| %{ case couponDiscountPercent coupon }
| %{ of Just percent }
span.couponSimple_abstraction-summary-sub \#{percent}% OFF
| %{ of Nothing }
| %{ endcase }

This is more verbose than just having a maybe control statement.

Of course, you could write it without the of Nothing part like the following, but the compiler might give a warning about an "incomplete pattern match":

| %{ case couponDiscountPercent coupon }
| %{ of Just percent }
span.couponSimple_abstraction-summary-sub \#{percent}% OFF
| %{ endcase }

Can you think of a better way to write it than this? ↑

The downside of the maybe control statement is that it only works for the Maybe data type. It doesn't work for the Either datatype, or any other sum type.

@arowM Do you want to implement the case and elseif control statements? If not, I could start implementing them tomorrow!

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 9, 2017

I agree using case instead of maybe makes template more verbose.
But is removing few lines so important?
If it is always so, all of programmers would use ruby as their main programming languages ;P

I believe the important thing we have to consider when we decide to accept a new syntax or not is:

  1. How the new syntax makes our template file type safe?
  2. How the new syntax makes our template file easy to read for front-end developers?
    • Heterocephalus syntaxes are mainly written by front-end developers.
  3. Is the new syntax will not cause a bad know how?

I think that if maybe control statement was acceptable, it is because maybe has a lot of benefit regarding to the second point.

In general, adding new feature to a library is easy, but removing is hard because removing loses backward compatibility.
It is the reason I do not decide to accept maybe control statement now.

@cdepillabout, if you have any thought that maybe statement have real benefit like case control statement, please tell me.

Do you want to implement the case and elseif control statements? If not, I could start implementing them tomorrow!

Feel free to implement them. I'm so happy if you do so.
And if you can, could you update documents as well?
I know it makes documents better than I do :)

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 9, 2017

Okay, that sounds good.

if you have any thought that maybe statement have real benefit like case control statement, please tell me.

I agree that the maybe control statement doesn't have any real benefit, unlike the case control statement. I think the only benefit of the maybe control statement is that it could make the code a little shorter. But that is because it is specialized to work on only the Maybe datatype and not any other sum type.

So I agree that for now we shouldn't add the maybe control statement.

Feel free to implement them. I'm so happy if you do so. And if you can, could you update documents as well?

I'll start working on it today.

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 14, 2017

I just submitted PR #13 adding support for the elseif control statement. It was actually pretty easy to support because of #12.

Tomorrow I'll work on adding support the case control statement. I think this should take a little more time because I'll actually have to modify the Text.Heterocephalus module.

@arowM I had a question for you. How do you go about actually checking the code that has been generated when you are doing development?

For example, when adding support for the case control statement, I will need to generate some Haskell code (using Haskell's case expression), but I'm not sure the best way to go about actually making sure the code I'm generating is correct. How do you go about checking?

I know about the -ddump-splices compiler flag for dumping template-haskell code, but I'm wondering if there is a better way?

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 14, 2017

Great, @cdepillabout . I'll check the PR later.
Because lots of codes are from Text.Hamlet.hs, which already have case statement, it might be easy to copy some of code from Text.Hamlet.hs.
(Of course, it is much better, if they exported parsing functions...)

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 14, 2017

How do you go about actually checking the code that has been generated when you are doing development?

To tell the truth, I'm not sure, too.
I only check the parsed types (Control and Doc) are correct or not in my hands, and trust doctest tests about compile* functions.
But it might not so important to check template-haskell-generated codes, because the core functions about template-haskell are copied from mature Hamlet module.

@cdepillabout , If you have any good plan to check generated codes such as -ddump-splices and believe it's worse to spend some times, please feel free to propose in independent PR or issue.

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 14, 2017

In #13, I've added an example executable that can easily be used to check the generated Haskell code. I'll use it when making sure that I've implemented the case control statements correctly.

@chreekat

This comment has been minimized.

Copy link

@chreekat chreekat commented Jan 16, 2017

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 16, 2017

Thanks for the suggestion @chreekat!

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 16, 2017

I've created a short blog post and github repo explaining how to use -ddump-splices for debugging:

https://functor.tokyo/blog/2017-01-16-looking-at-generated-template-haskell

This is spurred by the discussion for #13.

@arowM

This comment has been minimized.

Copy link
Owner

@arowM arowM commented Jan 23, 2017

I've released v1.0.3.0, which contains commits about case control statements.

@cdepillabout

This comment has been minimized.

Copy link
Contributor Author

@cdepillabout cdepillabout commented Jan 23, 2017

Thanks, I'll close this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.