search

gjtorikian / html-pipeline

HTML processing filters and utilities
MIT License
2.27k stars 382 forks source link

`rescue LoadError; abort` flow for missing dependencies breaks client applications #240

Closed parkr closed 8 years ago

parkr commented 8 years ago

Just left a comment in https://github.com/jch/html-pipeline/pull/80#issuecomment-171727303, but wanted to surface it in a new issue. Ever since a0acb6956fdede396b814ce695eef78af0d56b96 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 is abort necessary?

Real-life example: https://github.com/gjtorikian/jekyll-html-pipeline/pull/7#issue-126687668

/cc @jch @simeonwillbanks who collaborated on #80.

jch commented 8 years ago

The reasoning for abort is we want to fail early with a custom error message explaining what the developer needs to do. The alternative is that the dependency is not met, and the load error is raised, which was confusing.

More background in my reply in https://github.com/gjtorikian/jekyll-html-pipeline/pull/7#issuecomment-171731073

parkr commented 8 years ago

we want to fail early with a custom error message explaining what the developer needs to do.

From what I saw, this was accomplished with a custom require.

The alternative is that the dependency is not met, and the load error is raised, which was confusing.

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.

jch commented 8 years ago

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.

parkr commented 8 years ago

@jch Is there any way to modify the behaviour while still using errors? abort in a library makes it really difficult to work with that library, especially when filters are required dynamically.

jch commented 8 years ago

Why is abort necessary?

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