-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Better error when using route helpers incorrectly #1372
Better error when using route helpers incorrectly #1372
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big area of confusion for me, and rendering an error which is more helpful than Crystal will help a ton. For me it goes like this:
- Edit the
need
of an action (ohh how easy it is to just add aneed
! just so easy! spill the bucket of red paint on the floor) - Edit "the page that uses the action"
- See this opaque error
- Open up terminal and start grepping for other uses of the ActionName
- Clean up the mess with a mop until the floor is clean enough to compile again
{% end %}{% if !optionals.empty? %} | ||
Optional arguments: | ||
{% for opts in optionals %}\n- {{ opts.id }}{% end %} | ||
{% end %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this list be formatted more like a method call, or terminal documentation?
A stab in the dark: {{ @type }}.with( {{ required things }}, [ {{ optional things }} ] )
or something? It's confusing to me to see the arguments broken out in a bulleted list like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into doing something like this but I'd be afraid that if I put that in the error message that it would be an indication of the correct way to call it but the [ {{ optional }} ]
part is not right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments.
6a6ccf3
to
97d065b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will save me buckets of time, thanks @matthewmcgarvey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Purpose
Fixes #1005
Description
There's an implementation
Lucky::Routable.route
that handles correct usage but when the method is called incorrectly it provides a confusing error message. We're seeing people assume it was a problem in their action and not where the error actually is, where they try to create a link or something using the action class name.Before
After
I'm still kind of disappointed with the error message. I was hoping that by adding the compilation error it would point towards the usage site but it still doesn't.
Checklist
crystal tool format spec src
./script/setup
./script/test