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

Suggest possible solutions to failing requires #5487

Merged
merged 3 commits into from Apr 29, 2018

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Dec 30, 2017

See #5291

@straight-shoota
Copy link
Member

Maybe this could also display CRYSTAL_PATH to show the locations where the compiler can discover a lib.

@RX14
Copy link
Contributor Author

RX14 commented Dec 30, 2017

@straight-shoota that's not really useful information, it's just an implementation detail that shards and crystal have to agree on. Providing that information might lead users to stop thinking about shards and miss the solution to their problem.

@straight-shoota
Copy link
Member

straight-shoota commented Dec 30, 2017

It depends on how close the coupling between the compiler and shards is supposed to be. It is obviously the preferred tool and strongly recomended for Crystal development, but the compiler can do very well without shards. In fact, it doesn't need it at all. You can just install libraries manually in the lib path or use some other manager for this. I'm not sure if the compiler should just assume everyone uses shards.

Currently, the only reference to shards in the compiler is crystal deps (which should probably be deprecated). And crystal init creates shards.yml (but also a bunch of other, opinionated config files).

@RX14
Copy link
Contributor Author

RX14 commented Dec 30, 2017

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 RX14 mentioned this pull request Dec 30, 2017
6 tasks
@larubujo
Copy link
Contributor

rx14 is right here

crystal is shards ♪
shards is crystal ♫

please sing with this song: https://www.youtube.com/watch?v=7er_xx7Wmg8

#{error}

Looks like you're trying to require a shard.
- Did you remember to run `shards install`?
Copy link
Contributor

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 🎉

@straight-shoota
Copy link
Member

straight-shoota commented Dec 31, 2017

The compiler supports lookup paths like lib/foo.cr which cannot possibly be populated by shards install. It clearly does not declare shards to be the only way to use libraries.

That's why I don't think the error message should assume someone want's to require a shard just because require contains a non-relative path. It could also be very likely refer to a misspelled file in the stdlib or one that is not available in the current version of Crystal's stdlib.

I'd suggest to reword this to not reference "shard" explicitly. Maybe require a library?

It should certainly mention to try shards install, missing that is probably a very common source of error.

There should also be a list of the places where the compiler tried to locate that file.
The path in the error message is confusing: For require "foo" it says can't find file 'foo' relative to '/current/path/src'. While this is largely correct in principle (although the relative to is actually the project folder, not src), without a deeper understanding of the require mechanism no-one can possibly know what it means. An unknowing user could read this as it would resolve to /current/path/src/foo.cr. But if you put a file there, it won't be found because it's not a path-relative lookup and src is not in the crystal path.

It should better state that it can't find the file in CRYSTAL_PATH.

That relative to '/current/path/src' makes only sense for path-relative requires.

Users could identify possible issues easier if the compiler showed the actual paths where it looks for the file. Maybe shards install was run, but it was not done correctly (for example some directories got mixed up) or shards had some failure. If the paths are shown, a user may spot such issues easier.

@RX14
Copy link
Contributor Author

RX14 commented Dec 31, 2017

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.

Copy link
Member

@straight-shoota straight-shoota left a 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.

Copy link
Contributor

@ysbaddaden ysbaddaden left a 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.

@RX14
Copy link
Contributor Author

RX14 commented Dec 31, 2017

@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.

@asterite
Copy link
Member

asterite commented Jan 2, 2018

We could simply have the compiler depend on YAML (which means libyaml) and let the compiler parse shard.yml when such require fails. That is probably the best solution (maybe not the simplest).

(alternatively, port YAML to pure crystal, but that means more code to compile, so slower compile times)

I know this hardcodes the dependency of crystal with shards. But originally crystal deps was embedded in the compiler. We extracted it to ease development. But crystal and shards are meant to coexist and always be used together. shards is not just one package manager for crystal. It's the package manager.

@straight-shoota
Copy link
Member

I'm generally in favour of parsing shards.yml in the compiler. That doesn't even mean a dependency on shards itself, but only on the spec file. And a missing or incomplete file could be handled gracefully.

But what benefit would the knowledge of shards.yml bring in this specific use case? I don't think it improves much of the error message if we can say for certain that a shard is specified but not installed.

@RX14
Copy link
Contributor Author

RX14 commented Jan 2, 2018

@asterite surely that simply removes all the benefits of having crystal and shards seperate. Now the compiler depends on the layout and speciication of shard.yml. Depending on shards in terms of documentation and error messages is great because that's a very loose coupling that extracts the best from them being in different repositories. Parsing shard.yml is a very tight coupling and at that point you might as well make shards be part of the crystal repository.

@asterite
Copy link
Member

asterite commented Jan 2, 2018

It's a soft dependency. The compiler detects that require "foo" fails. Then:

  • if shards.yml exists:
  • and if it can be parsed as a YAML file
  • and it matches the structure expected by shards (there's a "dependencies" node)
  • and inside "dependencies" there's a "foo" node
    • then say "did you miss running shards install"
    • otherwise, give a generic error

There's no real dependency with shards, and not even requiring that shards.yml is present. But if it is, and foo is listed as a dependency, and require "foo" fails, then it's like 99.99% probable that the user is missing running shards install.

@straight-shoota
Copy link
Member

I'm not sure that's worth the effort for this small feature. I'm just guessing here, but I expect missing sharfs install to be a very common source of error. And honestly, there is no harm done to suggest it when it's actually a different issue. Maybe the dependency is missing from shards.ymlor just misspelled. Running shards install in this case will likely be a guide in the right direction, too, even if it does not immediately solve it.

@RX14
Copy link
Contributor Author

RX14 commented Jan 2, 2018

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.

@RX14
Copy link
Contributor Author

RX14 commented Jan 4, 2018

@ysbaddaden any further comment or are we at an impasse?

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Jan 8, 2018

Let's say we fail to require "stra/foo"; we can assume that we expected the "stra" library to have a foo.cr file (wherever it's located, we don't care). From this information, we can personalize the message, and provide a better help to the user.

For example glob available libraries (from CRYSTAL_PATH), see if "stra" library exist. If it doesn't exist maybe we could apply a Levenshtein distance, and propose (just like we do for types and methods):

"stra/foo.cr" not found, did you mean "star/foo"?

If a "stra" library exists, propose to upgrade the library with shards update, and hint to the possibility that the file no longer exists in the updated library, ...

I.e. we can do better, for everyone benefit, not just respond to an isolated beginner issue with a generic message :)

@RX14
Copy link
Contributor Author

RX14 commented Jan 8, 2018

@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.

@ysbaddaden
Copy link
Contributor

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.

@RX14
Copy link
Contributor Author

RX14 commented Jan 14, 2018

Is there anything left to do without spending the time to write in another feature?

@RX14
Copy link
Contributor Author

RX14 commented Jan 20, 2018

ping?

@RX14 RX14 merged commit db0dc67 into crystal-lang:master Apr 29, 2018
@RX14 RX14 added this to the Next milestone Apr 29, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this pull request Jun 6, 2018
* Suggest possible solutions to failing requires

* fixup! Suggest possible solutions to failing requires

* fixup! Suggest possible solutions to failing requires
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants