-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Suggest possible solutions to failing requires #5487
Conversation
Maybe this could also display |
@straight-shoota that's not really useful information, it's just an implementation detail that |
It depends on how close the coupling between the compiler and Currently, the only reference to |
The compiler should assume everyone uses shards, and it shouldn't confuse users by suggesting otherwise until there is an otherwise. Helping new users far outweighs theoretics about alternate package managers. |
rx14 is right here
please sing with this song: https://www.youtube.com/watch?v=7er_xx7Wmg8 |
src/compiler/crystal/crystal_path.cr
Outdated
#{error} | ||
|
||
Looks like you're trying to require a shard. | ||
- Did you remember to run `shards install`? |
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.
Perfect use-case for 1:25:16 🎉
The compiler supports lookup paths like That's why I don't think the error message should assume someone want's to I'd suggest to reword this to not reference "shard" explicitly. Maybe It should certainly mention to try There should also be a list of the places where the compiler tried to locate that file. It should better state that it can't find the file in CRYSTAL_PATH. That Users could identify possible issues easier if the compiler showed the actual paths where it looks for the file. Maybe |
The error message no longer contains "relative to" if it's not a relative require. I also changed the wording to say "If you're trying to require a shard". I'm not going to list the paths it searched because the 0.1% of users who ever do library management manually without shards will surely have read the docs. I contend it's actively confusing to users to receive a humongous list (can easily be 10+ entries long) of places the compiler has searched for a file. Users don't care where the file is, just how to get it there. |
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'd still like a reference to the paths but this is fine, too.
Maybe add a spec for relative to
.
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.
IMHO: too noisy, referring to shard.yml
implies a false logical correlation: oh, crystal is trying to find shards from parsing shard.yml
which is plain wrong (crystal searches for libraries installed under lib
and doesn't care about shard.yml
).
I'm sure we can do better. Messages can be explanatory, yet concise and correct. A mere seems enough:
Did you run 'shards install'? Did you run the compiler from your project root, not a subfolder?
Still, maybe a library was updated and the file i'm requiring was renamed/removed and I'm going to be misled by the 'helpful' message.
@ysbaddaden I actually don't think it matters what impression of the implementation details this message gives the users, I think it matters if it helps them solve their problem. Defining the project root as "the same directory as shard.yml" is a good easy way for new users to find the directory. |
We could simply have the compiler depend on (alternatively, port I know this hardcodes the dependency of crystal with shards. But originally |
I'm generally in favour of parsing But what benefit would the knowledge of |
@asterite surely that simply removes all the benefits of having crystal and shards seperate. Now the compiler depends on the layout and speciication of |
It's a soft dependency. The compiler detects that
There's no real dependency with |
I'm not sure that's worth the effort for this small feature. I'm just guessing here, but I expect missing |
Surely this message is far better than none. Programmers know to check for typos, and we should also trust them to know whether what they want is a shard or not. This message gives users enough hints about the very most common gotchas that new users trip up on when trying to use shards. Getting the perfect error message for every situation is clearly impossible, let's not try. |
@ysbaddaden any further comment or are we at an impasse? |
Let's say we fail to For example glob available libraries (from
If a "stra" library exists, propose to upgrade the library with I.e. we can do better, for everyone benefit, not just respond to an isolated beginner issue with a generic message :) |
@ysbaddaden yeah that's cool and all but that's an additional feature on top of this PR. I'd like to merge the featureset this PR provides before talking about extra work. Don't let perfect get in the way of better and all. |
I'm not convinced. Maybe Levenshtein can come later (maybe), but we can at least get the library name and file path, and customize the message with them, simplify the message, and get something good. |
Is there anything left to do without spending the time to write in another feature? |
ping? |
* Suggest possible solutions to failing requires * fixup! Suggest possible solutions to failing requires * fixup! Suggest possible solutions to failing requires
See #5291