luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

link_to route helper shows obscure error for missing required arguments #1652

Open grepsedawk opened 2 years ago

grepsedawk commented 2 years ago

Describe the bug When using the link_to macro with a path that has required attributes, the error provided is not very helpful. image

Then jumping through the full error trace, our code's call doesn't bring much information either: image

To Reproduce Steps to reproduce the behavior:

Example:

  1. Generate a show route
  2. Use show route without calling anything: link draw.to_s, to: ::Draws::Show

Expected behavior

Catch the error for the user, suggest that the required attributes might need to be called using .with on the ShowPage to pass the required arguments into the page

Screenshots/code If applicable, add screenshots/code samples to help explain your problem.

Versions (please complete the following information):

matthewmcgarvey commented 1 year ago
Screen Shot 2022-09-13 at 12 29 22 AM

Seems that the standard compiler error has changed now with Crystal v1.5.0.

Thinking about what would be expected and using my picture as an example... ideally it should point to the call to link saying that you can't pass LinkHelpers::Show and point at where link was called. I can't think of a way to accomplish that with the way things are right now. The most obvious thing to reach for would be to add an overload route method that takes no arguments to actions that have required parameters but I think that would have to provide a super generic error message and still not point to the line we want it to.

To think outside of the current implementation, we could have to have three different interfaces.

  1. Lucky::RouteHelper that's what you get when you provide parameters
  2. Lucky::Action the base... params could be required or not
  3. Lucky::ParamsNotRequiredAction layered onto the base Lucky::Action... params are not required

In the macro process of the action class, you'd extend the action with Lucky::ParamsNotRequiredAction if there are no required params. That module would give a route method that would be removed from Lucky::Action. At least in the Lucky::LinkHelpers module you'd replace the methods that take in Lucky::Action with Lucky::ParamsNotRequiredAction. That way, if you pass in an action that requires params without providing them, it would fail to compile exactly where we want it to. The only issue is, it wouldn't quite explain itself as we'd like so we'd likely have to provide an implementation of the methods with Lucky::Action and provide a clearer error message.

This is all just off the top of my head, so I have no idea if it'd even work. I'll look into it more as I'm interested in it right now.

jwoertink commented 1 year ago

I wonder what this error message will look like in Crystal 1.6 with the errors changing https://github.com/crystal-lang/crystal/pull/12469