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.

Sign up for GitHub

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

Jump to bottom

Fixing Crystal 1.13 regression issues #1900

Merged
merged 3 commits into from
Jun 28, 2024
Merged

Fixing Crystal 1.13 regression issues #1900

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6dbbf43
Overriding Habitat so we can test against Crystal 1.13 fixes
jwoertink Jun 15, 2024
38fc80c
fixing regression issues for Crystal 1.13. Fixes #1872
jwoertink Jun 15, 2024
8714877
actually use the shard override to do shard override. probably time t…
jwoertink Jun 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions shard.override.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
dependencies:
habitat:
github: luckyframework/habitat
branch: main
6 changes: 4 additions & 2 deletions src/lucky/assignable.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  # ...
end

This starts to compound when you're talking about hundreds of actions, models, queries, operations, pages, etc...

} ||
!dec.value.is_a?(Nop)
has_explicit_value ? 1 : 0
} %}
Expand All @@ -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? %}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤷‍♂️

@{{ var.id }} : {{ type }}{% if !value.is_a?(Nop) %} = {{ value }}{% end %},
{% end %}
**unused_exposures
Expand Down
2 changes: 1 addition & 1 deletion src/lucky/routable.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ module Lucky::Routable
end

{% params_with_defaults = PARAM_DECLARATIONS.select do |decl|
!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?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here resolve.nilable? should be ok 🤞 .... but if it starts blowing up apps, then we know where to come back to..

end %}
{% params_without_defaults = PARAM_DECLARATIONS.reject do |decl|
params_with_defaults.includes? decl
Expand Down
Loading