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

Update dependencies for Jekyll 3 #7

Merged
merged 6 commits into from
Jan 14, 2016
Merged

Update dependencies for Jekyll 3 #7

merged 6 commits into from
Jan 14, 2016

Conversation

benbalter
Copy link
Collaborator

Because all the other HTML Pipeline-based gems require HTML PIpeline >= 2.

I'm getting one failure, that I'm not sure how to fix:

  1) Failure:
HTMLPipelineTest#test_fail_when_a_library_dependency_is_not_met [/Users/benbalter/projects/jekyll-html-pipeline/test/test_jekyll_html_pipeline.rb:30]:
LoadError expected but nothing was raised.

@parkr
Copy link
Collaborator

parkr commented Jan 14, 2016

It looks like this was broken by gjtorikian/html-pipeline@a0acb69.

@jch
Copy link

jch commented Jan 14, 2016

@parkr @benbalter not sure how to fix your failing test here. html-pipeline has talked about splitting apart the gem so that filters are in separate gems to manage dependencies (gjtorikian/html-pipeline#48). e.g. html-pipeline-whitelist would be it's own gem with it's on gemspec. But this couldn't be done without a major version release.

It's not ideal, but I would recommend updating the test to ensure whatever dependency you are looking for is being required.

@gjtorikian
Copy link
Owner

Thanks for this patch. The test is correctly failing:

  def test_fail_when_a_library_dependency_is_not_met
    override = @config.dup
    override['html_pipeline']['filters'] << 'AutolinkFilter'
    markdown = Jekyll::Converters::Markdown.new override
    assert_raises(LoadError) { markdown.convert('http://www.github.com') }
  end

The test should raise a LoadError because it shouldn't expect AutolinkFilter to work, because this gem should be missing rinku. Neither this gem nor the original html-pipeline provide dependencies for filters, even the standard filters. Downstream projects should request dependencies so that the pipeline works; this is inline with the discussion @jch linked to.

Could you please restore rinku and gemoji to the way they were?

@parkr
Copy link
Collaborator

parkr commented Jan 14, 2016

The test should raise a LoadError because it shouldn't expect AutolinkFilter to work, because this gem should be missing rinku.

html-pipeline v2 uses abort instead of raise, so no LoadError should be raised in html-pipeline v2, only in < 2.

@gjtorikian
Copy link
Owner

@benbalter, are you willing to modify the test so that it behaves differently based on the html-pipeline version used? Should be a simple check against HTML::Proofer::VERSION. If not, no worries, I'll open a PR myself for it. Just let me know.

@benbalter
Copy link
Collaborator Author

Could you please restore rinku and gemoji to the way they were?

sure thing. done.

are you willing to modify the test so that it behaves differently based on the html-pipeline version used?

Take a look at d9dec1c. It passes locally. I think I saw the "missing Rinku dependency" error in the output, and assumed it was my doing.

@gjtorikian
Copy link
Owner

@benbalter Looks good, thanks. Let's 💀 the 1.9.3 requirement and get a passing test:

@gjtorikian
Copy link
Owner

Thanks!

gjtorikian added a commit that referenced this pull request Jan 14, 2016
Update dependencies for Jekyll 3
@gjtorikian gjtorikian merged commit 6299b50 into gjtorikian:master Jan 14, 2016
@benbalter benbalter deleted the jekyll-3 branch January 14, 2016 19:49
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

Successfully merging this pull request may close these issues.

4 participants