Improve error message when returning non Lucky::Response #1086

paulcsmith · 2020-04-05T01:57:36Z

~~Closes~~ #460 #1086 (comment)
Closes #1037 and adds examples

paulcsmith · 2020-04-05T02:18:56Z

src/lucky/renderable.cr

+          else
+            redirect Home::Index
+          end
+        end


Added a specific example for when there is a nil returned

Same ❤️

paulcsmith · 2020-04-05T02:19:11Z

src/lucky/renderable.cr

+          html IndexPage, users: UserQuery.new
+          redirect to: Home::Index
+          json({name: "James"})
+        end


Added an example here too

I liked your realistic example above instead of showing all possible options. I think showing a different example could be good, but I think it should still work if it's an example. 🙂

Yeah good point!

paulcsmith · 2020-04-05T02:27:16Z

src/lucky/renderable.cr

+    # This is a hack to force Crystal to regenerate this method for
+    # each action. That way @type refers to the action with the problem,
+    # and not the first action to be compiled.
+    {% @type %}


Thanks @asterite for the tip! And thanks @jwoertink for digging into this

This is not needed because type is mentioned a bit below.

I think type need to be mentioned in the method that calls this method.

in perform_action

🤦‍♂ I happened to check this with an action that worked with the broken version. Thanks @asterite. Will give that a shot

Hmm this caused a compiler bug:

BUG: trying to downcast SignUps::New <- BrowserAction (Exception) web | from Crystal::CodeGenVisitor#downcast_distinct<LLVM::Value, Crystal::Type+, Crystal::Type+>:NoReturn

But I think I had an idea that might work...we'll see :P

src/lucky/renderable.cr

edwardloveall · 2020-04-05T13:02:02Z

src/lucky/renderable.cr

+          else
+            redirect Home::Index
+          end
+        end


edwardloveall · 2020-04-05T13:04:10Z

src/lucky/renderable.cr

+          html IndexPage, users: UserQuery.new
+          redirect to: Home::Index
+          json({name: "James"})
+        end


I liked your realistic example above instead of showing all possible options. I think showing a different example could be good, but I think it should still work if it's an example. 🙂

jwoertink

Sweet! It'll be nice to have this in. Especially in larger apps. Since it's hard to test macros, if it all works for you, then I'm good with it. 👍

jwoertink · 2020-04-05T16:25:28Z

src/lucky/renderable.cr

    {%
      raise <<-ERROR


-      Your action returned #{T}
+      #{@type} returned #{T}


Isn't this what we had before and it didn't work? 🤔

Yeah I was being dumb https://github.com/luckyframework/lucky/pull/1086/files#r403641754

And I couldn't figure out a way to get around it because there was a crystal bug in the compiler. Doing what Ary suggested did work when you didn't return a Lucky::Response, but when you the compiler blew up 🤷‍♂ So I guess this is still not possible. I think I'll keep the other changes and leave the other issue open for one day when we can do what we'd like or figure out a workaround :P

jwoertink · 2020-04-05T16:27:07Z

src/lucky/renderable.cr

+      For example...
+
+        get "/users" do
+          # Render html, json, or redirect


Suggested change

# Render html, json, or redirect

# Return one of html, json, or redirect

jwoertink · 2020-04-05T16:27:37Z

src/lucky/renderable.cr

+          else
+            redirect Home::Index
+          end
+        end


Same ❤️

paulcsmith

Thanks for the reviews!

paulcsmith · 2020-04-05T17:06:41Z

src/lucky/renderable.cr

    {%
      raise <<-ERROR


-      Your action returned #{T}
+      #{@type} returned #{T}


Yeah I was being dumb https://github.com/luckyframework/lucky/pull/1086/files#r403641754

And I couldn't figure out a way to get around it because there was a crystal bug in the compiler. Doing what Ary suggested did work when you didn't return a Lucky::Response, but when you the compiler blew up 🤷‍♂ So I guess this is still not possible. I think I'll keep the other changes and leave the other issue open for one day when we can do what we'd like or figure out a workaround :P

src/lucky/renderable.cr

paulcsmith · 2020-04-05T17:07:16Z

src/lucky/renderable.cr

+      For example...
+
+        get "/users" do
+          # Render html, json, or redirect


paulcsmith · 2020-04-05T17:07:37Z

src/lucky/renderable.cr

+          html IndexPage, users: UserQuery.new
+          redirect to: Home::Index
+          json({name: "James"})
+        end


Yeah good point!

Closes #1037 And improves error message

paulcsmith force-pushed the pcs/460-readd-helpful-message branch from 0b418b3 to c50cf85 Compare April 5, 2020 02:17

paulcsmith commented Apr 5, 2020

View reviewed changes

paulcsmith requested review from edwardloveall and jwoertink April 5, 2020 02:19

paulcsmith commented Apr 5, 2020

View reviewed changes

edwardloveall approved these changes Apr 5, 2020

View reviewed changes

jwoertink approved these changes Apr 5, 2020

View reviewed changes

paulcsmith commented Apr 5, 2020

View reviewed changes

paulcsmith force-pushed the pcs/460-readd-helpful-message branch 2 times, most recently from 07caa51 to 3f1dc36 Compare April 5, 2020 17:24

Improve error message when returning non Lucky::Response

91b736e

Closes #1037 And improves error message

paulcsmith force-pushed the pcs/460-readd-helpful-message branch from 3f1dc36 to 91b736e Compare April 5, 2020 17:25

paulcsmith merged commit 24613d9 into master Apr 5, 2020

paulcsmith deleted the pcs/460-readd-helpful-message branch April 5, 2020 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message when returning non Lucky::Response #1086

Improve error message when returning non Lucky::Response #1086

paulcsmith commented Apr 5, 2020 •

edited

Loading

paulcsmith Apr 5, 2020

edwardloveall Apr 5, 2020

jwoertink Apr 5, 2020

paulcsmith Apr 5, 2020

edwardloveall Apr 5, 2020

paulcsmith Apr 5, 2020

paulcsmith Apr 5, 2020

asterite Apr 5, 2020

asterite Apr 5, 2020

paulcsmith Apr 5, 2020

paulcsmith Apr 5, 2020 •

edited

Loading

edwardloveall Apr 5, 2020

edwardloveall Apr 5, 2020

jwoertink left a comment

jwoertink Apr 5, 2020

paulcsmith Apr 5, 2020

jwoertink Apr 5, 2020

paulcsmith Apr 5, 2020

jwoertink Apr 5, 2020

paulcsmith left a comment

paulcsmith Apr 5, 2020

paulcsmith Apr 5, 2020

paulcsmith Apr 5, 2020

	# Render html, json, or redirect
	# Return one of html, json, or redirect

Improve error message when returning non Lucky::Response #1086

Improve error message when returning non Lucky::Response #1086

Conversation

paulcsmith commented Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith Apr 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcsmith commented Apr 5, 2020 •

edited

Loading

paulcsmith Apr 5, 2020 •

edited

Loading