search

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

Cleaner Lucky::Action.route errors #1637

Closed matthewmcgarvey closed 2 years ago

matthewmcgarvey commented 2 years ago

In the spirit of recent discussion about simplifying macros and making errors more clear, I started looking back into where we have existing complications. For instance in https://github.com/luckyframework/lucky/pull/1372 we added a new method with a compiler error to help people avoid confusion when using Lucky::Action.route. We are still seeing people get confused about it, so I went to see if I could help make it even better. I started looking into moving the method and was about to make a PR with it and decided just to double check, to run the error without the overload to get a screenshot of the default error message and show how my change made it "better". I was totally surprised to find out that the default error was WAY better than what we have.

The Caveat

The problem is... the better default error message only appears if we remove Lucky::Action.with. If it's not taken out, the error message is still confusing.

Example

This is the line with the error ("foo" shouldn't be there but technically it's duplicated arguments since no_default is the first argument)

ParamsWithDefaultParamsLast.route("foo", no_default: "Yay!")

Specifying default incorrectly

Current
Screen Shot 2022-01-03 at 9 56 18 PM
Without custom `Lucky::Action.route` compiler error
Screen Shot 2022-01-03 at 9 57 11 PM
Without `Lucky::Action.with`
Screen Shot 2022-01-03 at 9 58 25 PM

There's multiple other ways to trigger a compilation error and all of them are clearly pointing at the correct line instead of within macro code. But are we willing to accept the cost? DUN-DUN-DUUUUN!!!

matthewmcgarvey commented 2 years ago

Technically, we might not have to remove Lucky::Action.with if we add all the correct parameters instead of the splat args... but we still could