Sorting the type declarations from needs so the nillable ones are las… #993
Conversation
…t in the constructor. Fixes #992
|
I don't think this goes far enough. The problem is not just with nilable types, there is still a problem if you have needs macro that has a default value before a needs statement that doesn't. In other words the following will fail just due to the order of the needs statements: needs some_var : String = "whatever"
needs required_var : String |
|
Ah, good call. Luckily the implementation is easy now, so adding on to it shouldn't be too bad. |
Moved the test spec to assignable where other needs are tested
| @@ -41,11 +43,23 @@ class PageWithQuestionMark | |||
| end | |||
| end | |||
|
|
|||
| class PageWithDefaultsFirst | |||
| include Lucky::HTMLPage | |||
| needs extra_css : String? | |||
There was a problem hiding this comment.
Could you add another needs above this one without a default and not nil? I’m not sure but I think that may show an issue
There was a problem hiding this comment.
And maybe a default of false too
There was a problem hiding this comment.
Adding a required one above this and the spec still passes.
There was a problem hiding this comment.
Ok cool! What about the false default?
There was a problem hiding this comment.
Cool. I added a few more just for some added comfort. They look good!
src/lucky/html_builder.cr
Outdated
| @@ -45,8 +45,9 @@ module Lucky::HTMLBuilder | |||
|
|
|||
| macro generate_needy_initializer | |||
| {% if !@type.abstract? %} | |||
| {% sorted_assigns = ASSIGNS.sort_by { |dec| dec.type.resolve.nilable? || dec.value ? 1 : 0 } %} | |||
There was a problem hiding this comment.
I think something like this may work and ensure that false and nil defaults work too:
decl.value || decl.value == nil || decl.value == false
I’m not sure but that’s my hunch
Also I don’t think it needs the type.resolve anymore since it just needs to sort defaults
There was a problem hiding this comment.
If the type is thing : String?, then there's no value at all. It doesn't seem to even equate to nil from what I can tell. Without the resolve, it won't compile. But I'll add in the check for it having a value of nil and false.
…also moved to the back of the list
| @@ -41,11 +43,23 @@ class PageWithQuestionMark | |||
| end | |||
| end | |||
|
|
|||
| class PageWithDefaultsFirst | |||
| include Lucky::HTMLPage | |||
| needs extra_css : String? | |||
There was a problem hiding this comment.
Ok cool! What about the false default?
| @@ -45,8 +45,11 @@ module Lucky::HTMLBuilder | |||
|
|
|||
| macro generate_needy_initializer | |||
| {% if !@type.abstract? %} | |||
| {% sorted_assigns = ASSIGNS.sort_by { |dec| | |||
| dec.type.resolve.nilable? || dec.value || dec.value == nil || dec.value == false ? 1 : 0 | |||
There was a problem hiding this comment.
I think this will work without the resolve now. That’d also be preferable because resolve can lead to weird loading issues like we used to have in models where you had to require models depending on alphabetic order
There was a problem hiding this comment.
Actually maybe not...since that would break the type union when it’s nilable with no default. So I guess this has to stay 👍 my bad
|
I think this looks good. Just trying to think if there’s a way to do it without resolving the type. But we can try to optimize for that later! LGTM |
…ope that's all of them
|
Sweet. Once these checks pass, I'll merge it in! |
Purpose
Fixes #992
Description
When you use
needsthat have both nilable and required values, the nilable ones had to come last otherwise there was a crystal compile error. This PR sorts the assignable type declarations putting the nilable ones last for the initialize method.cc. @bdtomlin @wezm
It'd be great if y'all could look over this and see if there's any other cases this solution doesn't cover.
Checklist
crystal tool format spec src./script/setup./script/test