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 to maruku 0.7 #1775

Merged
merged 3 commits into from Dec 7, 2013

Conversation

Projects
None yet
6 participants
@Ivoz
Contributor

Ivoz commented Dec 5, 2013

I do not believe there's been any major API change, AFAIK

Maruku Changelog

Gemspec now asks for 0.7.0 (final), which has been released.

It has changed some output behaviour after transitioning to its own output engine.

The parser will now do a .strip to take out any newlines Maruku has added surrounding html produced.

Maruku now produces " for attribute (src) quotes and uses the !CDATA tag inside script so that it will produce valid XHTML.

I updated the tests to account for these.

@Ivoz Ivoz referenced this pull request Dec 5, 2013

Closed

Trying out Maruku 0.7.0 #1558

0 of 2 tasks complete
@mattr-

This comment has been minimized.

Member

mattr- commented Dec 5, 2013

The parser will now do a .strip to take out any newlines Maruku has added surrounding html produced.

Can you take this out? It's a change we've rejected before, I believe.

@Ivoz

This comment has been minimized.

Contributor

Ivoz commented Dec 5, 2013

It's a change we've rejected before, I believe.

I'd love to know when/where (to see why)?

AFAIK html should never be invalidated / structurally changed by removing such newlines between full elements.

Otherwise you get these test failures:

  1) Failure:
test: An extracted excerpt #content after render should be the first paragraph of the page. (TestExcerpt) [/home/ivo/code/jekyll-dev/test/test_excerpt.rb:69]:
<"<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link ref</a>.</p>"> expected but was
<"\n<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link ref</a>.</p>\n">.

  2) Failure:
test: filters should markdownify with simple string. (TestFilters) [/home/ivo/code/jekyll-dev/test/test_filters.rb:25]:
<"<p>something <strong>really</strong> simple</p>"> expected but was
<"\n<p>something <strong>really</strong> simple</p>\n">.

  3) Failure:
test: A Post initializing posts rendering should include templates. (TestPost) [/home/ivo/code/jekyll-dev/test/test_post.rb:507]:
<"<<< <hr />\n<p>Tom Preston-Werner github.com/mojombo</p>\n\n<p>This <em>is</em> cool</p> >>>"> expected but was
<"<<< <hr />\n<p>Tom Preston-Werner github.com/mojombo</p>\n\n<p>This <em>is</em> cool</p>\n >>>">.

  4) Failure:
test: A Post processing posts #excerpt should return rendered HTML. (TestPost) [/home/ivo/code/jekyll-dev/test/test_post.rb:287]:
<"<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link ref</a>.</p>"> expected but was
<"\n<p>First paragraph with <a href=\"http://www.jekyllrb.com/\">link ref</a>.</p>\n">.

Which imho altering for would just be testing for maruku-specific output

An alternative would be to strip the test case output instead, I guess

@mattr-

This comment has been minimized.

Member

mattr- commented Dec 5, 2013

Oh, so the newlines are something new added in Maruku 0.7.0? If that's the case, then let's leave it in.

@@ -252,7 +252,7 @@ def fill_post(code, override = {})
end
should "write script tag" do
assert_match "<script src='https://gist.github.com/#{@gist}.js'>\s</script>", @result
assert_match "<script src='https://gist.github.com/#{@gist}.js'><![CDATA[\s]]></script>", @result

This comment has been minimized.

@parkr

parkr Dec 5, 2013

Member

This new CDATA declaration is kind of strange. I wonder if it interferes with the gist visually.

This comment has been minimized.

@Ivoz

Ivoz Dec 6, 2013

Contributor

Maruku 0.7 added them as they are technically needed to surround inline javascript for valid XHTML, which is what it aims to produce.

This comment has been minimized.

@parkr

parkr Dec 6, 2013

Member

Ok, cool! I don't this will be a blocker so 👍

@parkr

This comment has been minimized.

Member

parkr commented Dec 5, 2013

I ❤️ the call to String#strip for sure. Anything to reduce the change in behaviour, the better.

As long as 100% of the tests pass, I'm 👍.

This supercedes #1558.

@konklone

This comment has been minimized.

Contributor

konklone commented Dec 5, 2013

Also 👍, for what that's worth. Thanks for bringing this home.

@parkr

This comment has been minimized.

Member

parkr commented Dec 6, 2013

Ok so I can't see any of the build histories on GitHub (wtf?), luckily TravisCI gives a listing of PRs and their build histories.

@mattr- I'm 👍 on this, as tests are passing and there aren't any incompatibilities from a site perspective that I can think of.

@zoul

This comment has been minimized.

zoul commented Dec 6, 2013

FWIW, Maruku 0.7 also contains a fix for an undesired shortening of HTML tags that makes Jekyll screw up the output with some simple markup.

mattr- added a commit that referenced this pull request Dec 7, 2013

@mattr- mattr- merged commit 9f15932 into jekyll:master Dec 7, 2013

@Ivoz

This comment has been minimized.

Contributor

Ivoz commented Dec 7, 2013

Awesome, cheers

@mattr-

This comment has been minimized.

Member

mattr- commented Dec 7, 2013

Updated history in 8b0ea62

@konklone

This comment has been minimized.

Contributor

konklone commented Dec 8, 2013

Woo!

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.