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

Rewritten PHP lexer makes kramdown and jekyll tests fail #1603

Closed
dleidert opened this issue Oct 8, 2020 · 5 comments
Closed

Rewritten PHP lexer makes kramdown and jekyll tests fail #1603

dleidert opened this issue Oct 8, 2020 · 5 comments
Labels
bugfix-request A request for a bugfix to be developed.

Comments

@dleidert
Copy link

dleidert commented Oct 8, 2020

When I run the jekyll and ruby-kramdown tests with the latest rouge release some tests fail and it points to the PHP lexer. Here is the output:

In jekyll:

Failure:
Minitest::Result#test_: with the rouge highlighter post content has highlight tag with linenumbers should should stop highlighting at boundary with rouge 2.  [/<<PKGBUILDDIR>>/test/test_tags.rb:482]
Minitest::Assertion: Expected /<p>This\ is\ not\ yet\ highlighted<\/p>\n\n<figure\ class="highlight"><pre><code\ class="language\-php"\ data\-lang="php"><table\ class="rouge\-table"><tbody><tr><td\ class="gutter
\ gl"><pre\ class="lineno">1\n<\/pre><\/td><td\ class="code"><pre><span\ class="nx">test<\/span>\n<\/pre><\/td><\/tr><\/tbody><\/table><\/code><\/pre><\/figure>\n\n<p>This\ should\ not\ be\ highlighted,\ right\?
<\/p>\n/ to match "<hr />\n<p>title: This is a test\n—</p>\n\n<p>This is not yet highlighted</p>\n\n<figure class=\"highlight\"><pre><code class=\"language-php\" data-lang=\"php\"><table class=\"rouge-table\"><t
body><tr><td class=\"gutter gl\"><pre class=\"lineno\">1\n</pre></td><td class=\"code\"><pre><span class=\"n\">test</span>\n</pre></td></tr></tbody></table></code></pre></figure>\n\n<p>This should not be highlig
hted, right?</p>\n".

Note the class="nx" vs class="n" outout.

In kramdown:

  1) Failure:
TestFiles#test_/<<BUILDDIR>>/ruby-kramdown-2_3_0/test/testcases/block/06_codeblock/rouge/simple_text_to_html [/<<PKGBUILDDIR>>/test/test_files.rb:56]:
--- expected
+++ actual
@@ -5,7 +5,7 @@
 </code></pre>
 </div></div>
 
-<div class=\"language-php highlighter-rouge\"><div class=\"highlight\"><pre class=\"highlight\"><code><span class=\"nv\">$foo</span> <span class=\"o\">=</span> <span class=\"k\">new</span> <span class=\"nx\">Ba
r</span><span class=\"p\">;</span>
+<div class=\"language-php highlighter-rouge\"><div class=\"highlight\"><pre class=\"highlight\"><code><span class=\"nv\">$foo</span> <span class=\"o\">=</span> <span class=\"k\">new</span> <span class=\"nc\">Ba
r</span><span class=\"p\">;</span>
 </code></pre>
 </div></div>
 "


  2) Failure:
TestFiles#test_/<<BUILDDIR>>/ruby-kramdown-2_3_0/test/testcases/block/06_codeblock/rouge/multiple_text_to_html [/<<PKGBUILDDIR>>/test/test_files.rb:56]:
--- expected
+++ actual
@@ -6,7 +6,7 @@
 </code></pre>
 </div></div></div>
 
-<div class=\"language-php highlighter-rouge\"><div class=\"custom-class\"><div class=\"highlight\"><pre class=\"highlight\"><code><span class=\"nv\">$foo</span> <span class=\"o\">=</span> <span class=\"k\">new<
/span> <span class=\"nx\">Bar</span><span class=\"p\">;</span>
+<div class=\"language-php highlighter-rouge\"><div class=\"custom-class\"><div class=\"highlight\"><pre class=\"highlight\"><code><span class=\"nv\">$foo</span> <span class=\"o\">=</span> <span class=\"k\">new<
/span> <span class=\"nc\">Bar</span><span class=\"p\">;</span>
 </code></pre>
 </div></div></div>
 "

Note the class="nx" vs class="nc" output.

Name of the lexer
It points to the php lexer, but I'm not sure.

Code sample
Test cases as provided by jekyll and kramdown.

@dleidert dleidert added the bugfix-request A request for a bugfix to be developed. label Oct 8, 2020
@pyrmont
Copy link
Contributor

pyrmont commented Oct 8, 2020

@dleidert I've only looked at this quickly but aren't the differences here just the class names? It looks to me like the Rouge classes are more accurate.

@dleidert
Copy link
Author

dleidert commented Oct 8, 2020

If rouge is working correctly then these tests need to be adjusted and I have to report it to the jekyll and kramdown projects. So the question is: is rouge working correctly?

@pyrmont
Copy link
Contributor

pyrmont commented Oct 8, 2020

Rouge might well have a bug but errors in Jekyll's tests are a matter for Jekyll. If you have a bug with the way Rouge is highlighting something, please provide a sample of the code being highlighted incorrectly (especially helpful if you can link to it in the Dingus).

@dleidert
Copy link
Author

dleidert commented Oct 8, 2020

The tests expect a behavior and rouge changed it (tests work stable up to rouge 3.21). And the question is if this is intentional. If yes then the tests need adjusting to pick up the new behavior. But still the rouge project also has responsible to provide a stable behavior.

@pyrmont
Copy link
Contributor

pyrmont commented Oct 9, 2020

@dleidert Rouge lexers are sometimes updated to more accurately lex input code. That will necessarily result in different output. We don't view this as a breaking change but do mark these as new feature-level changes (and update the version accordingly).

The PHP lexer was updated in v3.22.0 to improve its output (#1489). If there's input that it's lexing incorrectly, please let us know the specific input and we can look at correcting this behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix-request A request for a bugfix to be developed.
Projects
None yet
Development

No branches or pull requests

2 participants