Place Dependency Management On Filters

[simeonwillbanks]

#48 kickstarted discussion, and here is a plan for placing dependency management on Filters.

1. Add dependency management tests
2. Add dependency management to Filter with descriptive exception
   message
3. Refactor Filters to use new dependency management logic
4. For CI, move gem dependencies from gemspec to Gemfile :test block
5. Add gem post install message alerting users to new dependency
   management
6. Update README to detail each Filters dependencies e.g. FaradayMiddleware README

[simeonwillbanks]

Here is a DependencyManagement mixin prototype. Its heavily influenced from Faraday::Middleware.

module HTML
  module DependencyManagement

    def dependency(path, gem=nil)
      require path
    rescue LoadError => error
      # http://ruby-doc.org/core-1.9.3/LoadError.html
      # Possibilities:
      # > require 'this/file/does/not/exist'
      # LoadError: no such file to load -- this/file/does/not/exist
      missing_gem = gem.nil? ? path : gem
      self.load_error = "missing dependency for #{self}: #{error.message}\n" +
        "add `gem '#{missing_gem}'` to your Gemfile, or `gem install #{missing_gem}`"
    rescue NameError => error
      # We rescue NameError incase the required dependency has another dependency
      # which has not been required e.g. `uninitialized constant AnotherDependency`
      #
      # http://ruby-doc.org/core-1.9.3/NameError.html
      # Possibilities:
      # > puts foo
      # NameError: undefined local variable or method `foo' for main:Object
      # > Fixnum.const_set :answer, 42
      # NameError: wrong constant name answer
      self.load_error = "unable to require #{path} dependency for #{self}: #{error.message}"
    end

    def new(*)
      raise load_error unless loaded?
      super
    end

    def loaded?
      load_error.nil?
    end

    def load_error
      @load_error
    end

    private

    def load_error=(load_error)
      @load_error = load_error
    end
  end
end

module HTML
  class Filter
    extend DependencyManagement
    dependency 'rake'
    dependency 'net/http/persistent', 'net-http-persistent'
  end
end

HTML::Filter.new

Here are a couple examples of messages raised.

dependency_management.rb:29:in `new': missing dependency for HTML::Filter: LoadError (RuntimeError)
add `gem 'net-http-persistent'` to your Gemfile, or `gem install net-http-persistent`
    from dependency_management.rb:57:in `<main>'

dependency_management.rb:29:in `new': unable to require net/http/persistent dependency for HTML::Filter: NameError (RuntimeError)
    from dependency_management.rb:57:in `<main>'

[jch]

I remember seeing Faraday's dependency management DSL. I'll sleep on it tonight, but I feel it's a bit overkill. Yes, it would save a few lines at the top of each filter class, but it's just a glorified begin..rescue block. Then again, having a module would also allow us to handle semver. Something like:

dependency 'rake', '~> 0.10'
dependency 'net-http-persistent', :require => 'net/http/persistent'

How do you feel about that? Even more overkill? By the way, however we release this change, I'll probably bump it to version 1.0. The library's been stable for a long time, and most pulls have been docs related. This would be a big enough change to push it over.

[simeonwillbanks]

I could agree with overkill. Here is a simplistic dependency management implementation from em-synchrony.

The proposed prototype DRYs the error messages, but you could just use a constant.

# filter.rb
module HTML
  class Pipeline
    NOTICE_MISSING_GEM = "Missing html-pipeline dependency: gem install %s"
  end
end

# foo_filter.rb
begin
  require "net/http/persistent"
rescue LoadError => error
  raise sprintf(HTML::Pipeline::NOTICE_MISSING_GEM, "net-http-persistent")
  # => "Missing html-pipeline dependency: gem install net-http-persistent"
end

module HTML
  class Foo < Filter
  end
end

I really like the DependencyManagement module because it clearly communicates what is broken and how to fix it. A developer sees more than an error message.

LoadError: no such file to load -- net/http/persistent

Plus, Filters with many dependencies will not repeat begin..rescue..end blocks.

That said, only lib/html/pipeline.rb has multiple requires, so the large majority of files will have a single begin..rescue..end block. Again, the constant clearly communicates the issue and steps to fix it.

Semver is super interesting, but it sounds like even more overkill. 😄

Awesome! 1.0 ✨

[simeonwillbanks]

Hmm, maybe we can integrate semver into the simple dependency management, at least the version syntax.

# filter.rb
module HTML
  class Pipeline
    NOTICE_MISSING_GEM = %Q(Missing html-pipeline dependency.\nPlease add `gem "%s", "%s"` to your Gemfile.)
  end
end

# foo_filter.rb
begin
  require "net/http/persistent"
rescue LoadError => error
  raise sprintf(HTML::Pipeline::NOTICE_MISSING_GEM, "net-http-persistent", "~> 2.9")
  # => "Missing html-pipeline dependency.\nPlease add `gem \"net-http-persistent\", \"~> 2.9\"` to your Gemfile."
end

module HTML
  class Foo < Filter
  end
end

Initially, we could hard-code the versions. For this release or a subsequent release, we could add a build task which introspects the Gemfile test block and writes the correct version to the appropriate file.

Thoughts?

[jch]

Here's some notes about the existing dependencies:

Required

• nokogiri - used to make DocumentFragment

Optional

• gemoji - EmojiFilter
• github-markdown - MarkdownFilter
• sanitize - SanitizationFilter
• rinku - AutolinkFilter
• escape_utils - EmailReplyFilter, PlainTextInputFilter
• activesupport - used for instrumentation, not required anywhere else as far as I know.

[rsanheim]

I'm 👎 on any sort of module or magic beyond a line or two of ruby to handle dependancies here. Feels like overkill given how simple our dependencies are for this stuff.

[simeonwillbanks]

After weighing the options, I agree with @rsanheim's and @jch's initial thoughts. Keep it simple.

begin..rescue..end blocks EmojiFilter, MarkdownFilter, SanitizationFilter, AutolinkFilter, EmailReplyFilter, PlainTextInputFilter. ( Thanks @jch for the list 🤘 )

What about the exception message? I prefer the clear message with steps to fix.

"Missing html-pipeline dependency: Please add `gem "github-markdown", "~> 0.5.3"` to your Gemfile."

[jch]

Sounds good! Looking forwards to the pull.

On Wednesday, August 21, 2013, Simeon Willbanks wrote:

After weighing the options, I agree with @rsanheimhttps://github.com/rsanheim's
and @jch https://github.com/jch's initial thoughts. Keep it simple.

begin..rescue..end blocks EmojiFilter, MarkdownFilter, SanitizationFilter,
AutolinkFilter, EmailReplyFilter, PlainTextInputFilter. ( Thanks @jchhttps://github.com/jchfor the list [image:
🤘] )

What about the exception message? I prefer the clear message with steps to
fix.

"Missing html-pipeline dependency: Please add gem "github-markdown", "~> 0.5.3" to your Gemfile."

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/77#issuecomment-23039326
.

-Jerry
@whatcodecraves http://twitter.com/whatcodecraves
github http://github.com/jch

[simeonwillbanks]

For sure. I'll get started.