Skip to content
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

Warn if "pipelines" are out of order. #47

Closed
envygeeks opened this issue Apr 1, 2013 · 4 comments
Closed

Warn if "pipelines" are out of order. #47

envygeeks opened this issue Apr 1, 2013 · 4 comments

Comments

@envygeeks
Copy link
Contributor

I would love it if rather then sending a generic error that means nothing to the user (in some cases) and could be confusing, html-pipeline should detect order issues if there is a clear process order or emoji should convert the DocumentFragment. What I mean is:

[
  HTML::Pipeline::MarkdownFilter,
  HTML::Pipeline::EmojiFilter
]

Works, but

[
  HTML::Pipeline::EmojiFilter,
  HTML::Pipeline::MarkdownFilter
]

Fails. However your lib sends people a broad message that doesn't even hint closely to what the problem might be, it only sends: https://github.com/jch/html-pipeline/blob/master/lib/html/pipeline/text_filter.rb#L7 which can confuse some users who are simply doing the most simple things like:

class HTMLPipeline < Filter
  FILTERS =
    [
      HTML::Pipeline::EmojiFilter
      HTML::Pipeline::MarkdownFilter,
    ]

  def run(content, opts = {})
    opts = { gfm: true, asset_root: "/assets/img" }.merge(opts)
    HTML::Pipeline.new(FILTERS, opts).to_html(content)
  end
end

This might be a problem with Emoji on Ruby 2.0.0-p0 though.

@jch
Copy link
Contributor

jch commented Apr 1, 2013

That's a nasty gotcha. The MarkdownFilter documentation does specify it needs to be the first filter in the pipeline, but this information needs to be more discoverable. We can pass in the Pipeline instance into the context hash for filters to do more introspection. Would you be interested in creating a pull for that?

@mtodd
Copy link
Contributor

mtodd commented Apr 1, 2013

One possibility would be to rescue TypeError inside the MarkdownFilter and issue a custom warning, reminding those that see it that the MarkdownFilter needs to be first in the pipeline (which isn't entirely accurate anyways: it just needs to be the last TextFilter since it changes the source from text to HTML).

@mtodd
Copy link
Contributor

mtodd commented Apr 1, 2013

@envygeeks I wrote up some quick recommended changes for your example: https://gist.github.com/mtodd/216223f2307634e78e7b

Let me know if you have any questions on normal usage.

@jch
Copy link
Contributor

jch commented Apr 1, 2013

@mtodd Thanks for helping out! 🍻

@jch jch closed this as completed Jul 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants