-
-
Notifications
You must be signed in to change notification settings - Fork 382
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
rescue LoadError; abort
flow for missing dependencies breaks client applications
#240
Comments
The reasoning for More background in my reply in gjtorikian/jekyll-html-pipeline#7 (comment) |
From what I saw, this was accomplished with a custom
The custom load message made sense to me. Are you saying the default behaviour of showing the stacktrace is confusing? I agree it can sometimes be too much information, but generally users know to look at the top of the blob they get back for info. |
I agree with ya, but that still didn't help with the issues people were filing. We also documented it in the readme https://github.com/jch/html-pipeline#dependencies. Even with this custom message, it's not ideal because we don't check against specific versions of the dependency, so there's unexpected failures if someone has an old gem too. |
@jch Is there any way to modify the behaviour while still using errors? |
@parkr sorry, I missed your original point ^^ You're totally right that we don't need abort. Would you or @simeonwillbanks be interested in opening a PR that raises a custom error? It could even use LoadError, but just customize the message. |
Just left a comment in #80 (comment), but wanted to surface it in a new issue. Ever since a0acb69 was released, it became impossible for client applications to handle missing dependencies. If you're a rails app using this and you don't have a dependency that someone is looking for, your process will abort, which is not going to make your users happy.
raise
-ing an error generally returns a 1 exit code. Why isabort
necessary?Real-life example: gjtorikian/jekyll-html-pipeline#7 (comment)
/cc @jch @simeonwillbanks who collaborated on #80.
The text was updated successfully, but these errors were encountered: