Fixing Crystal 1.13 regression issues #1900
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jwoertink
commented
Jun 16, 2024
| @@ -89,7 +89,9 @@ module Lucky::Assignable | |||
| {% sorted_assigns = ASSIGNS.sort_by { |dec| | |||
| has_explicit_value = | |||
| dec.type.is_a?(Metaclass) || | |||
| dec.type.types.map(&.id).includes?(Nil.id) || | |||
| dec.type.types.any? { |t| | |||
| (t.is_a?(Metaclass) || t.is_a?(ProcNotation) || t.is_a?(Generic)) ? false : t.names.includes?(Nil.id) | |||
There was a problem hiding this comment.
Not a fan of doing this, but if we use type.resolve here we will have to force all Lucky apps to start adding a bunch of require statements at the top of all the files because of examples like..
class PostPage < MainLayout
# in macro land, Crystal doesn't know what "User" is yet. If we resolve this type at compile time, we will have to require it at the top of this file.
needs user : User
# ...
endThis starts to compound when you're talking about hundreds of actions, models, queries, operations, pages, etc...
jwoertink
commented
Jun 16, 2024
| @@ -98,7 +100,7 @@ module Lucky::Assignable | |||
| {% var = declaration.var %} | |||
| {% type = declaration.type %} | |||
| {% value = declaration.value %} | |||
| {% value = nil if type.stringify.ends_with?("Nil") && !value %} | |||
| {% value = nil if type.stringify.ends_with?("Nil") && value.nil? %} | |||
There was a problem hiding this comment.
I changed this because if for some reason you did param thingable : Bool? = false it would default the value to nil. Probably a non-issue, but why not? 🤷♂️
jwoertink
commented
Jun 16, 2024
| !decl.value.is_a?(Nop) || decl.type.is_a?(Union) && decl.type.types.last.id == Nil.id | ||
| !decl.value.is_a?(Nop) || decl.type.is_a?(Union) && decl.type.resolve.nilable? |
There was a problem hiding this comment.
Here resolve.nilable? should be ok 🤞 .... but if it starts blowing up apps, then we know where to come back to..
…o clean up this CI workflow
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes #1872
Description
Crystal 1.13 changes how
Nilis written on the macro side. This broke some of the Lucky code because now::Nil.idis not the same asNil.id.Checklist
crystal tool format spec src./script/setup./script/test