search

gjtorikian / html-pipeline

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

Eating LoadErrors can mask different problems #282

Closed timdiggins closed 6 years ago

timdiggins commented 7 years ago

I had an incompatible version of nokogiri for sanitize installed in my local Gemfile.lock (for gem development, so not committed to repo).

The sanitization_filter.rb then rescued this LoadError and output the (incorrect) message that there was sanitize was missing dependency. see: https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/sanitization_filter.rb#L4

Took me some headscratching to work out what was going wrong.

This has been discussed before: https://github.com/jch/html-pipeline/pull/160#issuecomment-59421899

I think it would be a good idea to output (in every case of rescuing the LoadError) the original exceptions message...

Something as simple as the following perhaps? (per file)

begin
  require "sanitize"
rescue LoadError => e
  raise HTML::Pipeline::MissingDependencyError, "Missing dependency (or other LoadError) 'sanitize' for SanitizationFilter. See README.md for details.\n----\n#{e}"
end
timdiggins commented 7 years ago

So my point was -- do you need a test for all of these, or would you welcome a PR with just the changes?

gjtorikian commented 7 years ago

Just a PR with changes works. 👍

timdiggins commented 7 years ago

@gjtorikian I did a spec and a helper method in the end. Felt wrong modifying so many files with same changes and no tests.