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.

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

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

Open
grepsedawk opened this issue Jan 22, 2022 · 2 comments · May be fixed by #1734
Open

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

grepsedawk opened this issue Jan 22, 2022 · 2 comments · May be fixed by #1734
Assignees
Labels
improve error experience Make errors nicer to deal with

Comments

@grepsedawk
Copy link
Contributor

grepsedawk commented Jan 22, 2022

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):

  • Lucky version (check in shard.lock): 0.29.0
  • Crystal version (crystal --version): 1.3.2
  • OS: macOS
@grepsedawk grepsedawk added the bug label Jan 22, 2022
@grepsedawk grepsedawk changed the title route WIP: link_to route helper shows obscure error for missing required arguments Jan 22, 2022
@grepsedawk grepsedawk changed the title WIP: link_to route helper shows obscure error for missing required arguments link_to route helper shows obscure error for missing required arguments Jan 22, 2022
@jwoertink jwoertink added improve error experience Make errors nicer to deal with and removed bug labels Jan 23, 2022
@matthewmcgarvey
Copy link
Member

matthewmcgarvey commented Sep 13, 2022

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
Copy link
Member

I wonder what this error message will look like in Crystal 1.6 with the errors changing crystal-lang/crystal#12469

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve error experience Make errors nicer to deal with
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants