-
-
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
Filters Manage Dependencies #80
Changes from 1 commit
d3fc6d9
c25c890
f0983dc
f9c6d06
e042c3e
0471de4
3801fd5
154ef94
6ff25ad
a0acb69
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,24 @@ | ||
source 'https://rubygems.org' | ||
source "https://rubygems.org" | ||
|
||
# Specify your gem's dependencies in html-pipeline.gemspec | ||
gemspec | ||
|
||
group :development do | ||
gem 'bundler' | ||
gem 'rake' | ||
gem "bundler" | ||
gem "rake" | ||
end | ||
|
||
group :test do | ||
gem "rinku", "~> 1.7", :require => false | ||
gem "gemoji", "~> 1.0", :require => false | ||
gem "RedCloth", "~> 4.2.9", :require => false | ||
gem "escape_utils", "~> 0.3", :require => false | ||
gem "github-linguist", "~> 2.6.2", :require => false | ||
gem "github-markdown", "~> 0.5", :require => false | ||
|
||
if RUBY_VERSION < "1.9.2" | ||
gem "sanitize", ">= 2", "< 2.0.4", :require => false | ||
else | ||
gem "sanitize", "~> 2.0", :require => false | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would move these into their own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried platforms :ruby_18 do
gem "sanitize", ">= 2", "< 2.0.4", :require => false
end
platforms :ruby_19, :ruby_20 do
gem "sanitize", "~> 2.0", :require => false
end gem "sanitize", ">= 2", "< 2.0.4", :require => false, :platforms => :ruby_18
gem "sanitize", "~> 2.0", :require => false, :platforms => [:ruby_19, :ruby_20] Bundler doesn't like either way.
I think we should stick with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reference: rubygems/bundler#751 |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,18 +98,29 @@ filter.call | |
* `TextileFilter` - convert textile to html | ||
* `TableOfContentsFilter` - anchor headings with name attributes and generate Table of Contents html unordered list linking headings | ||
|
||
## Syntax highlighting | ||
## Dependencies | ||
|
||
Filter gem dependencies are not bundled; you must bundle the filter's gem | ||
dependencies. The below list details filters with dependencies. For example, | ||
`SyntaxHighlightFilter` uses [github-linguist](https://github.com/github/linguist) | ||
to detect and highlight languages. It isn't included as a dependency by default | ||
because it's a large dependency and | ||
[a hassle to build on heroku](https://github.com/jch/html-pipeline/issues/33). | ||
To use the filter, add the following to your Gemfile: | ||
to detect and highlight languages. To use the `SyntaxHighlightFilter`, | ||
add the following to your Gemfile: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice edit. |
||
|
||
```ruby | ||
gem 'github-linguist' | ||
``` | ||
|
||
* `AutolinkFilter` - `rinku` | ||
* `EmailReplyFilter` - `escape_utils` | ||
* `EmojiFilter` - `gemoji` | ||
* `MarkdownFilter` - `github-markdown` | ||
* `PlainTextInputFilter` - `escape_utils` | ||
* `SanitizationFilter` - `sanitize` | ||
* `SyntaxHighlightFilter` - `github-linguist` | ||
* `TextileFilter` - `RedCloth` | ||
|
||
_Note:_ See [Gemfile](https://github.com/jch/html-pipeline/blob/master/Gemfile) `:test` block for version requirements. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub markdown supports relative links now. So you can just do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do. |
||
|
||
## Examples | ||
|
||
We define different pipelines for different parts of our app. Here are a few | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,13 +15,17 @@ Gem::Specification.new do |gem| | |
gem.test_files = gem.files.grep(%r{^test}) | ||
gem.require_paths = ["lib"] | ||
|
||
gem.add_dependency "gemoji", "~> 1.0" | ||
gem.add_dependency "nokogiri", RUBY_VERSION < "1.9.2" ? [">= 1.4", "< 1.6"] : "~> 1.4" | ||
gem.add_dependency "github-markdown", "~> 0.5" | ||
gem.add_dependency "sanitize", RUBY_VERSION < "1.9.2" ? [">= 2", "< 2.0.4"] : "~> 2.0" | ||
gem.add_dependency "rinku", "~> 1.7" | ||
gem.add_dependency "escape_utils", "~> 0.3" | ||
gem.add_dependency "nokogiri", RUBY_VERSION < "1.9.2" ? [">= 1.4", "< 1.6"] : "~> 1.4" | ||
|
||
gem.add_development_dependency "activesupport", RUBY_VERSION < "1.9.3" ? [">= 2", "< 4"] : ">= 2" | ||
gem.add_development_dependency "github-linguist", "~> 2.6.2" | ||
|
||
gem.post_install_message = <<msg | ||
---------------------------------------------- | ||
Thank you for installing html-pipeline! | ||
Filter gem dependencies are no longer bundled. | ||
You must bundle Filter gem dependencies. | ||
See html-pipeline README.md for more details. | ||
https://github.com/jch/html-pipeline | ||
---------------------------------------------- | ||
msg | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice touch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, since we're not 1.x yet, I'd remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I always forget this is available. |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
require 'rinku' | ||
begin | ||
require "rinku" | ||
rescue LoadError => e | ||
missing = HTML::Pipeline::Filter::MissingDependencyException | ||
raise missing, missing::MESSAGE % "rinku", e.backtrace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's overkill to have a custom exception class here because we want to exit as early as possible. The stack trace useful because it's not a programming error, it's a installation error. I'd prefer having the error string directly and calling begin
require 'rinku'
rescue LoadError => e
$stderr.puts "Missing dependency 'rinku' for AutolinkFilter. See README.md for details"
exit 1
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can make this simpler:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✨ learn me something new today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
end | ||
|
||
module HTML | ||
class Pipeline | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
require 'emoji' | ||
begin | ||
require "gemoji" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd leave it alone in case the gem ever decides to load more things in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. |
||
rescue LoadError => e | ||
missing = HTML::Pipeline::Filter::MissingDependencyException | ||
raise missing, missing::MESSAGE % "gemoji", e.backtrace | ||
end | ||
|
||
module HTML | ||
class Pipeline | ||
|
@@ -51,4 +56,4 @@ def asset_root | |
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
# Public: Methods useful for testing Filter dependencies. All methods are class | ||
# methods and should be called on the TestingDependency class. | ||
# | ||
# Examples | ||
# | ||
# TestingDependency.temporarily_remove_dependency_by gem_name do | ||
# exception = assert_raise HTML::Pipeline::Filter::MissingDependencyException do | ||
# load TestingDependency.filter_path_from filter_name | ||
# end | ||
# end | ||
class TestingDependency | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting class, but feels heavy handed. Seeing how we're really just testing rescuing a load error, I'd rather skip these tests entirely rather than adding this large testing dependency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understandable. I wanted to somehow stub It was a fun exercise. 💦 |
||
# Public: Use to safely test a Filter's gem dependency error handling. | ||
# For a certain gem dependency, remove the gem's loaded paths and features. | ||
# Once these are removed, yield to a block which can assert a specific | ||
# exception. Once the block is finished, add back the gem's paths and | ||
# features to the load path and loaded features, so other tests can assert | ||
# Filter functionality. | ||
# | ||
# gem_name - The String of the gem's name. | ||
# block - Required block which asserts gem dependency error handling. | ||
# | ||
# Examples | ||
# | ||
# TestingDependency.temporarily_remove_dependency_by gem_name do | ||
# exception = assert_raise HTML::Pipeline::Filter::MissingDependencyException do | ||
# load TestingDependency.filter_path_from filter_name | ||
# end | ||
# end | ||
# | ||
# Returns nothing. | ||
def self.temporarily_remove_dependency_by(gem_name, &block) | ||
paths = gem_load_paths_from gem_name | ||
features = gem_loaded_features_from gem_name | ||
|
||
$LOAD_PATH.delete_if { |path| paths.include? path } | ||
$LOADED_FEATURES.delete_if { |feature| features.include? feature } | ||
|
||
yield | ||
|
||
$LOAD_PATH.unshift(*paths) | ||
$LOADED_FEATURES.unshift(*features) | ||
end | ||
|
||
# Public: Find a Filter's load path. | ||
# | ||
# gem_name - The String of the gem's name. | ||
# | ||
# Examples | ||
# | ||
# filter_path_from("autolink_filter") | ||
# # => "/Users/simeon/Projects/html-pipeline/test/helpers/../../lib/html/pipeline/autolink_filter.rb" | ||
# | ||
# Returns String of load path. | ||
def self.filter_path_from(filter_name) | ||
File.join(File.dirname(__FILE__), "..", "..", "lib", "html", "pipeline", "#{filter_name}.rb") | ||
end | ||
|
||
private | ||
# Internal: Find a gem's load paths. | ||
# | ||
# gem_name - The String of the gem's name. | ||
# | ||
# Examples | ||
# | ||
# gem_load_paths_from("rinku") | ||
# # => ["/Users/simeon/.rbenv/versions/1.9.3-p429/lib/ruby/gems/1.9.1/gems/rinku-1.7.3/lib"] | ||
# | ||
# Returns Array of load paths. | ||
def self.gem_load_paths_from(gem_name) | ||
$LOAD_PATH.select{ |path| /#{gem_name}/i =~ path } | ||
end | ||
|
||
# Internal: Find a gem's loaded features. | ||
# | ||
# gem_name - The String of the gem's name. | ||
# | ||
# Examples | ||
# | ||
# gem_loaded_features_from("rinku") | ||
# # => ["/Users/simeon/.rbenv/versions/1.9.3-p429/lib/ruby/gems/1.9.1/gems/rinku-1.7.3/lib/rinku.bundle", | ||
# "/Users/simeon/.rbenv/versions/1.9.3-p429/lib/ruby/gems/1.9.1/gems/rinku-1.7.3/lib/rinku.rb"] | ||
# | ||
# Returns Array of loaded features. | ||
def self.gem_loaded_features_from(gem_name) | ||
# gem github-markdown has a feature "github/markdown.rb". | ||
# Replace gem name dashes and underscores with regexp | ||
# range to match all features. | ||
gem_name_regexp = gem_name.split(/[-_]/).join("[\/_-]") | ||
|
||
$LOADED_FEATURES.select{ |feature| /#{gem_name_regexp}/i =~ feature } | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
require "test_helper" | ||
|
||
class HTML::Pipeline::EmailReplyFilterTest < Test::Unit::TestCase | ||
def test_dependency_management | ||
assert_dependency_management_error "email_reply_filter", "escape_utils" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor pet peeve, but it's nice to keep pull requests focused on the changes described. These formatting edits make it trickier to review PRs. Don't worry about it for this PR, but just a heads up 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry and sure thing!