-
-
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 5 commits
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,6 +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 | ||
|
||
## 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. For example, 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. |
||
|
||
## 3rd Party Extensions | ||
|
||
If you have an idea for a filter, propose it as | ||
|
@@ -107,19 +130,6 @@ built as an external gem. | |
|
||
* [html-pipeline-asciidoc_filter](https://github.com/asciidoctor/html-pipeline-asciidoc_filter) - asciidoc support | ||
|
||
|
||
## Syntax highlighting | ||
|
||
`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: | ||
|
||
```ruby | ||
gem 'github-linguist' | ||
``` | ||
|
||
## 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,15 @@ 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 "activesupport", RUBY_VERSION < "1.9.3" ? [">= 2", "< 4"] : ">= 2" | ||
gem.add_dependency "nokogiri", RUBY_VERSION < "1.9.2" ? [">= 1.4", "< 1.6"] : "~> 1.4" | ||
gem.add_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! | ||
You must bundle Filter gem dependencies. | ||
See html-pipeline README.md for more details. | ||
https://github.com/jch/html-pipeline#dependencies | ||
------------------------------------------------- | ||
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,7 @@ | ||
require "test_helper" | ||
|
||
class HTML::Pipeline::EmailReplyFilterTest < Test::Unit::TestCase | ||
def test_dependency_management | ||
assert_dependency "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!