add inferred nil to html_builder when type ends with ?

[bdtomlin]

Purpose

Allow the more intuitive way to specify optional "needs" in html builder. #804

Description

Instead of

needs user : User? = nil

you can now just specify

needs user : User?

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[paulcsmith]

Woo hoo 🎉

[wezm]

I think this change broke my code. I have a component:

class Shared::LayoutHead < BaseComponent
  needs page_title : String
  needs app_js : Bool
  needs admin : Bool
  needs extra_css : String?
  # This is used by the 'csrf_meta_tags' method
  needs context : HTTP::Server::Context
  needs categories : CategoryQuery

  def render
    head do
      utf8_charset
      title "#{@page_title} – Read Rust"
      css_link asset("css/app.css"), data_turbolinks_track: "reload"
      js_link asset("js/app.js"), defer: "true", data_turbolinks_track: "reload" if @app_js

      if @admin
        css_link asset("css/admin.css"), data_turbolinks_track: "reload"
        js_link asset("js/admin.js"), defer: "true", data_turbolinks_track: "reload"
      end
      meta name: "turbolinks-cache-control", content: "no-cache"
      csrf_meta_tags
      responsive_meta_tag

      @categories.each do |category|
        tag "link", href: RssFeed::Show.with(category.slug).url, rel: "alternate", title: "Read Rust - #{category.name}", type: "application/rss+xml"
      end

      meta content: "Read Rust collects interesting posts related to the Rust programming language.", name: "description"
      css_link dynamic_asset(@extra_css), data_turbolinks_track: "reload" if @extra_css
    end
  end
end

This previously compiled on Lucky 0.18.0. On 0.18.2 the compiler reports this error. I'm using a picture here because useful information is lost without the colour and emphasised text.

The issue is the addition of the inferred nil default value because now there is an argument with a default value that precedes ones without, which is not allowed (see this test in the Crystal repo).

Before this change there was no default value on the extra_css parameter so it compiled ok. It's possible to work around the error by reordering the needs calls so that ones that are non-nil come first but this does not seem very intuitive. Plus, the error is extremely unhelpful as it does not point to the actual file where the error is happening. It wasn't until I discovered the --error-trace argument to the compiler that I was able to work out what was going on. This seems like it might be something that would trip up folks new to the framework. This is was the error is without --error-trace:

There was a problem expanding macro 'generate_needy_initializer'

Code in macro 'finished'

 1 | generate_needy_initializer
     ^
Called macro defined in lib/lucky/src/lucky/html_builder.cr:46:3

 46 | macro generate_needy_initializer

Which expanded to:

 > 35 |
 > 36 |
 > 37 |            @context : HTTP::Server::Context,
                   ^
Error: argument must have a default value

[paulcsmith]

Thank you for the detailed error report! It sounds like this did indeed cause the breakage. I think what we need to do is sort the “needs” to put them at the end when there is a default nil value. I’ll be gone for a couple weeks but may be able to take a look after that, but if somebody wants to tackle this before me I would not be mad :) Also, Wesley, could you open a new issue for this so we can track it more easily?

…

> On Dec 22, 2019, at 7:28 PM, Wesley Moore ***@***.***> wrote:  I think this change broke my code. I have a component: class Shared::LayoutHead < BaseComponent needs page_title : String needs app_js : Bool needs admin : Bool needs extra_css : String? # This is used by the 'csrf_meta_tags' method needs context : HTTP::Server::Context needs categories : CategoryQuery def render head do utf8_charset title ***@***.***_title} – Read Rust" css_link asset("css/app.css"), data_turbolinks_track: "reload" js_link asset("js/app.js"), defer: "true", data_turbolinks_track: "reload" if @app_js if @admin css_link asset("css/admin.css"), data_turbolinks_track: "reload" js_link asset("js/admin.js"), defer: "true", data_turbolinks_track: "reload" end meta name: "turbolinks-cache-control", content: "no-cache" csrf_meta_tags responsive_meta_tag @categories.each do |category| tag "link", href: RssFeed::Show.with(category.slug).url, rel: "alternate", title: "Read Rust - #{category.name}", type: "application/rss+xml" end meta content: "Read Rust collects interesting posts related to the Rust programming language.", name: "description" css_link ***@***.***_css), data_turbolinks_track: "reload" if @extra_css end end end This previously compiled on Lucky 0.18.0. On 0.18.2 the compiler reports this error. I'm using a picture here because useful information is lost without the colour and emphasised text. The issue is the addition of the inferred nil default value because now there is an argument with a default value that precedes ones without, which is not allowed (see this test in the Crystal repo). Before this change there were no default value on the extra_css parameter so it compiled ok. It's possible to work around the error by reordering the needs calls so that ones that are non-nil come first but this does not seem very intuitive. Plus, the error is extremely unhelpful as it does not point to the actual file where the error is happening. It wasn't until I discovered the --error-trace argument to the compiler that I was able to work out what was going on. This seems like it might be something that would trip up folks new to the framework. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or unsubscribe.

[wezm]

Wesley, could you open a new issue for this so we can track it more easily?

👍 Done

[wezm]

I think what we need to do is sort the “needs” to put them at the end when there is a default nil value.

I can't actually do this as my MainLayout has needs current_user : User?, so anything that inherits from that layout and has a non-optional needs call hits the error, but the order can't be changed. I'm in no rush to upgrade, enjoy your holiday @paulcsmith.

[paulcsmith]

Thanks and you too! Also, sorry I wasn’t more clear. I meant that Lucky needs to sort the “needs” so that the user doesn’t have to worry about it. It should “just work” regardless of when you define “needs” Hope that makes more sense

…

On Dec 22, 2019, at 8:18 PM, Wesley Moore ***@***.***> wrote:  I think what we need to do is sort the “needs” to put them at the end when there is a default nil value. I can't actually do this as my MainLayout has needs current_user : User?, so anything that inherits from that layout and has a non-optional needs call hits the error, but the order can't be changed. I'm in no rush to upgrade, enjoy your holiday @paulcsmith. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[wezm]

Oh right, yep that makes sense.

[bdtomlin]

@paulcsmith

It looks like this is an issue with the order of needs in general. You cannot have a needs that has defaults before needs that don't have defaults.

Thanks for the great error report @wezm.

I'll create a PR for this.

[jwoertink]

@bdtomlin I actually started looking at this, but I'm stuck. Maybe you'll have an easier time then me?

I run in to this issue when trying to sort these
https://play.crystal-lang.org/#/r/89e3

I was trying to sort the ASSIGNS here
https://github.com/luckyframework/lucky/blob/master/src/lucky/html_builder.cr#L47-L59

I did have an alternate version where I created 2 arrays. 1 for required and 1 for optional. Then had 2 separate for loops. It wasn't pretty though... However, we may have to go that route?

[jwoertink]

oh, actually, I may have it. Got some help from the community 😄 I'll push this up in a second

[bdtomlin]

@jwoertink, nice. Thanks! I look forward to seeing your solution :)