resolves #1082 HTMLTable formatter delegates to format method of inner formatter

[mojavelinux]

resolves #1082

• HTMLTable formatter delegates to format method of inner formatter instead of span method
• append trailing newline if missing (but don't wrap as whitespace token)
• add test

[mojavelinux]

Note that this line wasn't doing anything:

@inner.span(Token::Tokens::Text::Whitespace, "\n") { |str| formatted << str }

I've restored the functionality of appending the newline, but I decided not to format it as whitespace. That makes the formatting inconsistent with how other newlines are handled.

[mojavelinux]

Is it really necessary to yield more than once? I'd like to write the yield statement as follows:

yield %(<table class="#{@table_class}"><tbody><tr><td class="#{@gutter_class} gl"><pre class="lineno">#{formatted_line_numbers}</pre></td><td class="#{@code_class}"><pre>#{formatted}</pre></td></tr></tbody></table>)

[pyrmont]

@mojavelinux This is separate to #1085, right? Will that have any impact on this?

[mojavelinux]

Yes, this is separate from #1085. However, the whole purpose of this change is so that it can be used in combination with the line-wise formatter.

I'd still like to avoid the multiple yields, so I'm interested in getting feedback on that aspect. Yielding multiple times just seems wasteful.

[pyrmont]

@mojavelinux I wonder if the multiple yields are a way of making the HTML easier to read without breaking the tests? When I try to use HEREDOC syntax with a single yield (or just insert newlines into your proposed single yield statement) the Rouge::Formatters::HTMLLinewise spec fails. What do you think?

[mojavelinux]

@pyrmont I'm fine with splitting up the string to make it easier to read, but the multiple calls to yield introduce a different contract. Whatever it's yielding to is now receiving disconnected fragments of HTML instead of something whole, making it virtually impossible to do anything meaningful with them. I think the HTML needs to be assembled first, then yielded. We're mixing style with function here.

[mojavelinux]

I'll make an attempt to yield once and verify we can still pass the tests.

[mojavelinux]

I've updated as we discussed and all tests are passing.

[pyrmont]

Thanks @mojavelinux :)

[ashmaroli]

@mojavelinux Is there a possibility that this introduced an extra newline at the end of the rendered output?

Failure:
TestTags#test_: with the rouge highlighter post content has highlight tag should render markdown with rouge with line numbers.  [/home/travis/build/jekyll/jekyll/test/test_tags.rb:151]
Minitest::Assertion:

Expected
  /<table\ class="rouge\-table"><tbody><tr><td\ class="gutter\ gl"><pre\ class="lineno">1\n<\/pre><\/td><td\ class="code"><pre>test<\/pre><\/td><\/tr><\/tbody><\/table>/

to match
  "<table class=\"rouge-table\"><tbody><tr><td class=\"gutter gl\"><pre class=\"lineno\">1\n</pre></td><td class=\"code\"><pre>test\n</pre></td></tr></tbody></table>".

[mojavelinux]

Yes, I believe so. It's this line: https://github.com/rouge-ruby/rouge/blob/master/lib/rouge/formatters/html_table.rb#L29. I don't remember whether that was supposed to be part of this PR or not. The rationale is that it's important for the end of the lines stay clean to allow postprocessing to operate on all lines in the same way. When the closing </pre> bumps up against the last line, it requires special processing. The browser always ignores this trailing newline. (Notice the lineno pre is already this way).

[mojavelinux]

I'd recommend updating the test in Jekyll to make this trailing newline optional in the assertion.

[ashmaroli]

The rationale is that it's important for the end of the lines stay clean to allow postprocessing to operate on all lines in the same way. When the closing </pre> bumps up against the last line, it requires special processing.

I see.. Thanks for the above insight.

[ashmaroli]

@mojavelinux One more question: What is ?\n supposed to be? I see it repeated multiple times. Are all of them the same object or can it be stashed in a class constant?

[pyrmont]

It's a character literal.

[mojavelinux]

...and it's effectively a constant.

[ashmaroli]

It's a character literal.

...and it's effectively a constant.

TIL 🥂 to both of you 😄